Thread: User management functionality patch [pgadmin4]

Hi Harshal,

PFA comments as below,

1) While running setup.py for the first time, We did not handle the case to create Standard user.

2) Save button gets enable only clicking on input text box

3) Remove delete button from sub-node

4) If current user changes its own name/email, main page should reflect new changes. 

Can be listed as TODO

--------------------------------

1) Erros description is missing, for example when have user test@test.com presnt and user try to add user same as test@test.com we get error as "Error during saving user" it should be descriptive like "ERRPR: User {XYZ} is already exists.."

2) There should be an option to delete multiple users at one shot.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi 

3) There should be an option to active/deactivate multiple users in one shot.

4) In-place editing in backgrid.

Regards,

Murtuza

--

Regards,

Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 27, 2016 at 7:02 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:

Hi,

PFA initial patch for User management functionality.

-- 

Harshal Dhumal

Software Engineer 

EenterpriseDB

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Hi Harshal,

Please find the review comments so far:

Issues:

1. The UI design doesn't look interactive, especially add new user button.

2. In requirements.txt, while adding new dependent library the name of library and version should be separate by '==' instead of '--'

3. Under File menu, "Users" menu item should not be visible for non-admin users. For now it is only disabled.

4. Change role of admin user to non-admin user, then navigate to File > Users. It doesn't show users in Dialog and Add user doesn't work. Instead it is better to reload the page. Please find attached screenshot.

5. The data grid content should fill the unnecessary gap above the footer.

6. The title of delete user confirmation dialog should be "Delete user ?", instead of "Delete User".

7. The Edit and Delete column shouldn't be sortable.

8. Columns 'creation time' & 'last modified' are missing.

9. Password should have validation of minimum number of characters.

Can be added into TODO list:

1. There is no way to search a user by name or email id.

2. Pagination should be there if there are large number of users.

3. There should be a way of notifying the user once its password is changed by any administrator.

4. We can add 'created by' column.

5. One administrator shouldn't have privilege to modify the details of other administrator.

This is the role of super admin.

Thanks

Surinder Kumar

On Fri, May 27, 2016 at 7:02 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:

Hi,

PFA initial patch for User management functionality.

-- 

Harshal Dhumal

Software Engineer 

EenterpriseDB

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Hi Harshal,

PFA comments as below,

1) While running setup.py for the first time, We did not handle the case to create Standard user.

2) Save button gets enable only clicking on input text box

3) Remove delete button from sub-node

4) If current user changes its own name/email, main page should reflect new changes. 

Can be listed as TODO

--------------------------------

1) Erros description is missing, for example when have user test@test.com presnt and user try to add user same as test@test.com we get error as "Error during saving user" it should be descriptive like "ERRPR: User {XYZ} is already exists.."

2) There should be an option to delete multiple users at one shot.

3) There should be an option to active/deactivate multiple users in one shot.

4) In-place editing in backgrid.

Regards,

Murtuza

--

Regards,

Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 27, 2016 at 7:02 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:

Hi,

PFA initial patch for User management functionality.

-- 

Harshal Dhumal

Software Engineer 

EenterpriseDB

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Yeah, I was about to reply on this too;

- Why do we need Marshmallow?

- Why do we need a role system? Perhaps in the future we might have shared servers, and roles that cannot modify them, but not now.

- Why is the editing not done in-grid? It's only a single line per record. Seems like an awful lot of code for what should be a simple backgrid implementation.

On Tue, May 31, 2016 at 8:42 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Harshal,

Please find the updated patch with some improvement, we should not show any of the functions to the non-administrator user.

* Please handle unauthorised routes call using our own wrapper function.

   Because - role_required wrapper redirect the call to unauthorised page, which we don't required. We needed proper ajax response.

* Add search filter on top of grid.

* Open the form of user, when we add new user.

* Do not allow to add new user, if the form is not complete for the new user.

* Do not allow to close the form of new user with incomplete data. (Though - allow it to remove it.)

* Close the form on save button call.

* Allow to change attributes of existing users from the grid itself.

 

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

On Tue, May 31, 2016 at 12:12 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:

Hi,

-- 

Harshal Dhumal

Software Engineer 

EenterpriseDB

On Mon, May 30, 2016 at 4:09 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Harshal,

PFA comments as below,

1) While running setup.py for the first time, We did not handle the case to create Standard user.

Fixed

 

2) Save button gets enable only clicking on input text box

Fixed
 

3) Remove delete button from sub-node

Removed button.

4) If current user changes its own name/email, main page should reflect new changes. 

Moved to TODO

 

Can be listed as TODO

--------------------------------

1) Erros description is missing, for example when have user test@test.com presnt and user try to add user same as test@test.com we get error as "Error during saving user" it should be descriptive like "ERRPR: User {XYZ} is already exists.."

Fixed. (Added client side validation)

 

2) There should be an option to delete multiple users at one shot.

Moved to TODO
 

3) There should be an option to active/deactivate multiple users in one shot.

Moved to TODO

 

4) In-place editing in backgrid.

Moved to TODO

 

Regards,

Murtuza

--

Regards,

Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 27, 2016 at 7:02 PM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote:

Hi,

PFA initial patch for User management functionality.

-- 

Harshal Dhumal

Software Engineer 

EenterpriseDB

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 31, 2016 at 1:26 PM, Dave Page <dpage@pgadmin.org> wrote:

Yeah, I was about to reply on this too;

- Why do we need Marshmallow?

- Why do we need a role system? Perhaps in the future we might have shared servers, and roles that cannot modify them, but not now.

Hmm.. 

He just added one more role - 'Standard'.

'Administrator' role was already presents in it.

On Tue, May 31, 2016 at 9:52 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Tue, May 31, 2016 at 1:26 PM, Dave Page <dpage@pgadmin.org> wrote:

Yeah, I was about to reply on this too;

- Why do we need Marshmallow?

- Why do we need a role system? Perhaps in the future we might have shared servers, and roles that cannot modify them, but not now.

Hmm.. 

He just added one more role - 'Standard'.

'Administrator' role was already presents in it.

Oh yes, sorry, misread that. 'pgAdmin User Role' would be a better description though I think. 

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 31, 2016 at 2:45 PM, Dave Page <dpage@pgadmin.org> wrote:

On Tue, May 31, 2016 at 9:52 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Tue, May 31, 2016 at 1:26 PM, Dave Page <dpage@pgadmin.org> wrote:

Yeah, I was about to reply on this too;

- Why do we need Marshmallow?

- Why do we need a role system? Perhaps in the future we might have shared servers, and roles that cannot modify them, but not now.

Hmm.. 

He just added one more role - 'Standard'.

'Administrator' role was already presents in it.

Oh yes, sorry, misread that. 'pgAdmin User Role' would be a better description though I think. 

Yeah - that can be done. :-)

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Harshal,

Dave asked to put the User Management menu under the 'Change Password' (right top side).

On Fri, Jun 3, 2016 at 11:37 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Harshal,

Dave asked to put the User Management menu under the 'Change Password' (right top side).

Correct - that way it won't be displayed in desktop mode. Other comments:

- If I type an email address, then hit tab to select a Role, I immediately get an error saying that the role cannot be empty and the control loses focus. I should be able to add a user using just the keyboard.

- In general I'm seeing validation errors before I've had a chance to enter values. I should only see them if the appropriate field has lost focus.

- Role names should be "Administrator" and "User".

- The Close button should be disabled if errors are present.

- If I enter all the details for a new user and then hit Close, the dialog is closed and the new user is NOT added. I have to click something else first so the row loses focus, and then click close.

- The styling of the Add button and header does not match other grids (blue background for the header, title in white on the left, grey "ADD" button with no icon. The search box should be pushed right, to the left of the button as well I think.

- s/Search by email/Filter by email

- s/New Password/New password

- s/Confirm Password/Confirm password

- The font on the Close button doesn't match what's on the properties dialogues (looks like a global issue)

- The minimum size of the dialogue should be set such that the dialogue is usable at mimimum size, e.g. all columns shown, with 2 rows.

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

On Fri, Jun 3, 2016 at 10:52 PM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
>
> PFA attached patch (V5) for user management functionality.
>
> Note: If you've applied any of the previous patch of this functionality then
> set ConfigDB value to 10 in version table of and also delete role 'Standard'
> from role table before applying this patch.

Done - also restarted my app server, and done a hard refresh of the browser...

And I get "(index):310 Uncaught TypeError: Cannot read property
'show_users' of undefined" when I try to open the Users menu option.

>> - The Close button should be disabled if errors are present.
>
>
> I'm not convinced that to deny superuser from closing dialog for his
> mistakes (accidental mistakes).
>
> Consider a case when superuser clears email for any old user inadvertently
> (obviously this won't reflect on server). At this point there is no proper
> way that he can roll back or close the dialog without saving it if we
> disable close button. He has to either enter correct email for that user or
> refresh the browser.
>
> Another case while adding new user if he plans not to add user then he has
> to clear that partially filled user from grid before he can close the
> dialog.

Well we either need that, or a message box asking the user if he wants
to discard his changes and offering OK/Cancel options.

>> - If I enter all the details for a new user and then hit Close, the dialog
>> is closed and the new user is NOT added. I have to click something else
>> first so the row loses focus, and then click close.
>
>
> I was not able to reproduce this issue. I tried with both close buttons
> (top-right and bottom-right). Users were created in both the cases by adding
> all details and directly closing dialog without clicking anywhere on the
> dialog.

Hmm, I'll re-test when I get an updated patch.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

Thanks - I've commit as-is (with some minor tweaks), however the
following issues are present:

1) I get an error: "Invalid Email id: dpage+pg@pgadmin.org". That is a
perfectly valid email address, but I guess we're not recognising the
+.

2) When I mouse-over the Users menu option, the cursor isn't changing
to a pointer.

3) The font in the Close button is still not quite the same as for the
other dialogues.

Please fix and submit patch(es).

Thanks.

On Mon, Jun 6, 2016 at 11:58 AM, Harshal Dhumal

<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
> PFA updated patch (V6) for user management functionality.
>
> Changes: As per Ashesh's suggestion I have disabled email update of existing
> user.
>
>
> --
> Harshal Dhumal
> Software Engineer
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Mon, Jun 6, 2016 at 2:16 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> On Fri, Jun 3, 2016 at 10:52 PM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>> > Hi,
>> >
>> >
>> > PFA attached patch (V5) for user management functionality.
>> >
>> > Note: If you've applied any of the previous patch of this functionality
>> > then
>> > set ConfigDB value to 10 in version table of and also delete role
>> > 'Standard'
>> > from role table before applying this patch.
>>
>> Done - also restarted my app server, and done a hard refresh of the
>> browser...
>>
>> And I get "(index):310 Uncaught TypeError: Cannot read property
>> 'show_users' of undefined" when I try to open the Users menu option.
>
>
> This was an issue. Ideally Users menu shouldn't be visible to non admin
> users. I have fixed in this patch.
>
>>
>>
>> >> - The Close button should be disabled if errors are present.
>> >
>> >
>> > I'm not convinced that to deny superuser from closing dialog for his
>> > mistakes (accidental mistakes).
>> >
>> > Consider a case when superuser clears email for any old user
>> > inadvertently
>> > (obviously this won't reflect on server). At this point there is no
>> > proper
>> > way that he can roll back or close the dialog without saving it if we
>> > disable close button. He has to either enter correct email for that user
>> > or
>> > refresh the browser.
>> >
>> > Another case while adding new user if he plans not to add user then he
>> > has
>> > to clear that partially filled user from grid before he can close the
>> > dialog.
>>
>> Well we either need that, or a message box asking the user if he wants
>> to discard his changes and offering OK/Cancel options.
>
>
> I have added confirmation before closing dialog if any unsaved changes are
> present.
>
>
>>
>>
>> >> - If I enter all the details for a new user and then hit Close, the
>> >> dialog
>> >> is closed and the new user is NOT added. I have to click something else
>> >> first so the row loses focus, and then click close.
>> >
>> >
>> > I was not able to reproduce this issue. I tried with both close buttons
>> > (top-right and bottom-right). Users were created in both the cases by
>> > adding
>> > all details and directly closing dialog without clicking anywhere on the
>> > dialog.
>>
>> Hmm, I'll re-test when I get an updated patch.
>
> Ok
>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company