Обсуждение: PATCH: Column Level Privileges

On Thu, Jan 29, 2009 at 5:17 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Hi All,
>
> Please find the patch for Column Level Privileges support.
> I have also attached some of the test-cases for it.

Hi Ashesh,

Couple of minor issues/questions:

- Why didn't you derive the dialog from dlgSecurityProperty? I assume
it would have been too messy as it's already derived from
dlgTypeProperty?

- As a general rule, we disable controls not relevant to a particular
version of Postgres. We should do the same with this tab - disable the
controls rather than hide them on servers < 8.4.

- Can we lose the comment above the column permissions in the reverse
engineered SQL please? We don't add similar ones for other additional
queries so we shouldn't here (though perhaps in the future we might
add such comments everywhere that is appropriate).

- Not just an issue with your code, but also the existing privilege
tabs - could you please tweak the column sizes of the list control
such that the headers can be read by default?

Thanks!

--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

Couple of minor issues/questions:

- Why didn't you derive the dialog from dlgSecurityProperty? I assume
it would have been too messy as it's already derived from
dlgTypeProperty?

- As a general rule, we disable controls not relevant to a particular
version of Postgres. We should do the same with this tab - disable the
controls rather than hide them on servers < 8.4.

- Can we lose the comment above the column permissions in the reverse
engineered SQL please? We don't add similar ones for other additional
queries so we shouldn't here (though perhaps in the future we might
add such comments everywhere that is appropriate).

- Not just an issue with your code, but also the existing privilege
tabs - could you please tweak the column sizes of the list control
such that the headers can be read by default?

Thanks! 

- As a general rule, we disable controls not relevant to a particular
version of Postgres. We should do the same with this tab - disable the
controls rather than hide them on servers < 8.4.

- Can we lose the comment above the column permissions in the reverse
engineered SQL please? We don't add similar ones for other additional
queries so we shouldn't here (though perhaps in the future we might
add such comments everywhere that is appropriate).

- Not just an issue with your code, but also the existing privilege
tabs - could you please tweak the column sizes of the list control
such that the headers can be read by default?

Hi Ashesh

On Fri, Jan 30, 2009 at 6:48 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:

> Please find the updated patch.

Thanks - unfortunately, I found a couple of problems.

- If the dialog is the default (minimum) size when opened, the
privileges pane does not size correctly. Observed on Windows.

- If I open the table dialogue on an existing table, and then select a
column and click change. and then change an existing ACL entry, the
generated SQL includes a REVOKE ALL which revokes privileges from
other columns on the table.

/D

--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

- If the dialog is the default (minimum) size when opened, the
privileges pane does not size correctly. Observed on Windows.

- If I open the table dialogue on an existing table, and then select a
column and click change. and then change an existing ACL entry, the
generated SQL includes a REVOKE ALL which revokes privileges from
other columns on the table.

Hi

On Mon, Feb 2, 2009 at 12:55 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Hi Dave,
>
> Dave Page wrote:
>
> - If the dialog is the default (minimum) size when opened, the
> privileges pane does not size correctly. Observed on Windows.
>
> I could not reproduce it on windows (Vista). :(
> Could you please send a screen shot for it?

Attached before and after resize screenshots (it fixes itself if you
resize the dialogue)

> And also found one problem, when privileges are for group, "group " was not
> appended in privileges lists (ctlSecurityPanel). I found the same problem on
> existing privileges dialogs too. :(

Thats intentional - we dropped the 'group' keyword when roles were
introduced. It looks like your patch fixes a related issue with the
incorrect icon being used - we should keep that part of the fix :-)


--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

Hi

On Mon, Feb 2, 2009 at 12:55 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote: 

Hi Dave,

Dave Page wrote:

- If the dialog is the default (minimum) size when opened, the
privileges pane does not size correctly. Observed on Windows.

I could not reproduce it on windows (Vista). :(
Could you please send a screen shot for it?   

Attached before and after resize screenshots (it fixes itself if you
resize the dialogue) 

On Tue, Feb 3, 2009 at 10:04 AM, Ashesh D Vashi
<ashesh.vashi@enterprisedb.com> wrote:

> Please check, if this has solved the size problem.
> As I am not reproduce the same on my Vista, I can not verify it. :(
> But, I think - this should resolve the problem.

Nope, doesn't help. I'm surprised you cannot reproduce it on Vista
though - what version of wxWidgets are you using (I have 2.8.9)? Note
that the size is only incorrect if the dialog opens at the minimum
size - if it's any larger, it'll size correctly. You can open the
dialogue, shrink it as far it'll go, then close and re-open to see the
bug.

Guillaume; is this the issue you were adding a block of code to add
and then subtract a pixel from the height and width to fix?

--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

Dave Page a écrit :
> On Tue, Feb 3, 2009 at 10:04 AM, Ashesh D Vashi
> <ashesh.vashi@enterprisedb.com> wrote:
>
>> Please check, if this has solved the size problem.
>> As I am not reproduce the same on my Vista, I can not verify it. :(
>> But, I think - this should resolve the problem.
>
> Nope, doesn't help. I'm surprised you cannot reproduce it on Vista
> though - what version of wxWidgets are you using (I have 2.8.9)? Note
> that the size is only incorrect if the dialog opens at the minimum
> size - if it's any larger, it'll size correctly. You can open the
> dialogue, shrink it as far it'll go, then close and re-open to see the
> bug.
>

The way to reproduce it is this one:

 * open the dialog
 * resize it to its minimum size
 * close it
 * reopen it

And the UI issue should appear.

> Guillaume; is this the issue you were adding a block of code to add
> and then subtract a pixel from the height and width to fix?
>

Yes, that's right. If that's the only thing that prevents this patch
from being commited, commit it and I'll take a look at the UI glitch.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

On Tue, Feb 3, 2009 at 4:14 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:

> Yes, that's right. If that's the only thing that prevents this patch
> from being commited, commit it and I'll take a look at the UI glitch.

Thanks - committed.

And thanks to Ashesh for the patch!

--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

Dave Page a écrit :
> On Tue, Feb 3, 2009 at 4:14 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>
>> Yes, that's right. If that's the only thing that prevents this patch
>> from being commited, commit it and I'll take a look at the UI glitch.
>
> Thanks - committed.
>

I don't find the UI issue. Did you already fix it?


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com