public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikhil Mohite <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin][RM-6143]: Shared server entries not getting deleted.
Date: Mon, 22 Feb 2021 12:51:30 +0530
Message-ID: <CAOBg0ANTT=SeeG8yMaVUK_5UcO3fCGAae5KdzXAKL2GP3_kGeg@mail.gmail.com> (raw)
In-Reply-To: <CANxoLDe20GoJZjjcEsuACtGAAx5CaH2gEnC4N+V8M19UagfTVw@mail.gmail.com>
References: <CAOBg0APfo=oZvAotP9dpDf8_CveOnP9EpwsV=zn1uYPWgqPZ-Q@mail.gmail.com>
<CANxoLDfTspipwkMcJnNrnOWvguOMeHqNestMJ1dBqLGoowTeqQ@mail.gmail.com>
<CA+OCxowLS7AuXSmrfZ+PkOiVAiXU0RFPiJo8qz1b97yXG48mvA@mail.gmail.com>
<CANxoLDfN2AehHnBPDLPO1vR+yw-16T8dcRx04--UX-69UmCzqQ@mail.gmail.com>
<CAOBg0AM3ycNtnMwJ2Rk1Ty-qCW4dCsq11Zz6Eaai3bRWDb3LMg@mail.gmail.com>
<CA+OCxozsMp+p62THjyi=UX9j-4ZmsDz75OLYQPsPFWzQ6cEXQA@mail.gmail.com>
<CAOBg0ANns7UBMCXkuYQ-Rzsz4rtgQTCLOPke=ZMb77hkRq2fLg@mail.gmail.com>
<CA+OCxoygL=vjY73=sRd9c_aKcqfTTFw4OUJsGtD=0qJAZCTpXg@mail.gmail.com>
<CAOBg0AMA7tYj7zgOO0A4NVCPedq9ef9bWrg819ACFoTDgkhu-Q@mail.gmail.com>
<CANxoLDe20GoJZjjcEsuACtGAAx5CaH2gEnC4N+V8M19UagfTVw@mail.gmail.com>
Hi Akshay/ Team,
Please find an attached updated patch for RM-6143, Added missing updated
shared server owner name in the *"sharedserver"* table while the user
changing the ownership of the shared server.
On Thu, Feb 4, 2021 at 11:34 AM Akshay Joshi <[email protected]>
wrote:
> Thanks, patch applied.
>
> On Wed, Feb 3, 2021 at 2:22 PM Nikhil Mohite <
> [email protected]> wrote:
>
>> Hi Team,
>>
>> Please find the updated patch for RM-6143.
>>
>>
>> Regards,
>> Nikhil Mohite.
>>
>> On Mon, Feb 1, 2021 at 2:52 PM Dave Page <[email protected]> wrote:
>>
>>> Hi
>>>
>>> On Mon, Feb 1, 2021 at 7:51 AM Nikhil Mohite <
>>> [email protected]> wrote:
>>>
>>>> Hi Dave/ Team,
>>>>
>>>> As per the suggestions I have created a sample UI for change ownership
>>>> of shared servers before deleting the user(only for admin user).
>>>> [image: shared_server_change_ownsership.png]
>>>> The user list will contain all admin users. (This alert will get
>>>> display if the admin user has created any shared servers.)
>>>>
>>>
>>> The UI is fine, but the wording needs work. A couple of general hints:
>>> An entire sense would never be in parentheses, and there should always be a
>>> space following a full stop.
>>>
>>> "Select the user that will take ownership of the shared servers created
>>> by <user name>. <num servers> shared servers are currently owned by this
>>> user."
>>>
>>> "Note: If no user is selected, the shared servers will be deleted."
>>>
>>> I'd also suggest that if the user does not select a new owner, a message
>>> box should ask for confirmation before continuing:
>>>
>>> "The shared servers owned by <user name> will be deleted. Do you wish to
>>> continue?".
>>>
>>>
>>>>
>>>> Any suggestions or if I missed anything please let me know.
>>>>
>>>> Regards,
>>>> Nikhil Mohite
>>>>
>>>> On Thu, Jan 21, 2021 at 4:07 PM Dave Page <[email protected]> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Thu, Jan 21, 2021 at 10:33 AM Nikhil Mohite <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> On Thu, Jan 21, 2021 at 3:24 PM Akshay Joshi <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>> Reverted the commit.
>>>>>>>
>>>>>>> On Thu, Jan 21, 2021 at 3:13 PM Dave Page <[email protected]> wrote:
>>>>>>>
>>>>>>>> This seems like a very bad idea. What if the user that has left was
>>>>>>>> the user that setup 50 connections used by everyone else?
>>>>>>>>
>>>>>>>> Deleting those shared entries is (I would guess) most likely *not*
>>>>>>>> what the majority of users would want, and the current behaviour is
>>>>>>>> definitely safest.
>>>>>>>>
>>>>>>> In the current implementation when the admin user gets deleted all "*Server
>>>>>> groups*" created by that user are getting deleted, so if that admin
>>>>>> has created any *Shared server* other users are not able to access
>>>>>> it as its server group is not present in the database.
>>>>>>
>>>>>
>>>>> That seems bad. I would suggest we only delete the group if there are
>>>>> no shared servers left in it that would become orphaned. Of course, in that
>>>>> case we'll also need to reassign ownership of the group.
>>>>>
>>>>>
>>>>>>
>>>>>>>> We should make this optional; i.e. ask the use if they want shared
>>>>>>>> servers created by the user to be deleted. If they say no, they should be
>>>>>>>> reassigned to another user; either the admin that's deleting the user, or
>>>>>>>> their choice of user (a little more complex of course, but more flexible).
>>>>>>>>
>>>>>>> In the shared server table, we are creating entries per user, for
>>>>>> deletion of non-admin user we can delete the shared server tables entries
>>>>>> as it will not affect any other users. (because only admin users can mark
>>>>>> the server as shared.)
>>>>>> In case of admin user deletion, will add an extra check as suggested.
>>>>>>
>>>>>
>>>>> Sounds good - thanks!
>>>>>
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> Please revert this, until the deletion is made optional.
>>>>>>>>
>>>>>>>> On Thu, Jan 21, 2021 at 9:23 AM Akshay Joshi <
>>>>>>>> [email protected]> wrote:
>>>>>>>>
>>>>>>>>> Thanks, patch applied.
>>>>>>>>>
>>>>>>>>> On Thu, Jan 21, 2021 at 12:18 PM Nikhil Mohite <
>>>>>>>>> [email protected]> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Team,
>>>>>>>>>>
>>>>>>>>>> Please find the attached patch for RM-6143
>>>>>>>>>> <https://redmine.postgresql.org/issues/6143;: Shared server
>>>>>>>>>> entries not getting deleted.
>>>>>>>>>> Added code to delete shared server entries if the admin deletes
>>>>>>>>>> the user from user management.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> *Thanks & Regards,*
>>>>>>>>>> *Nikhil Mohite*
>>>>>>>>>> *Software Engineer.*
>>>>>>>>>> *EDB Postgres* <https://www.enterprisedb.com/;
>>>>>>>>>> *Mob.No: +91-7798364578.*
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> *Thanks & Regards*
>>>>>>>>> *Akshay Joshi*
>>>>>>>>> *pgAdmin Hacker | Principal Software Architect*
>>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>>
>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Dave Page
>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>> Twitter: @pgsnake
>>>>>>>>
>>>>>>>> EDB: http://www.enterprisedb.com
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> *Thanks & Regards*
>>>>>>> *Akshay Joshi*
>>>>>>> *pgAdmin Hacker | Principal Software Architect*
>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>
>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Nikhil Mohite.
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EDB: http://www.enterprisedb.com
>>>>>
>>>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EDB: http://www.enterprisedb.com
>>>
>>>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
> *pgAdmin Hacker | Principal Software Architect*
> *EDB Postgres <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>
Attachments:
[image/png] shared_server_change_ownsership.png (90.0K, 3-shared_server_change_ownsership.png)
download | view image
[application/octet-stream] RM_6143_v3.patch (1.7K, 4-RM_6143_v3.patch)
download | inline diff:
diff --git a/web/pgadmin/tools/user_management/__init__.py b/web/pgadmin/tools/user_management/__init__.py
index 15a1b673..ebfba540 100644
--- a/web/pgadmin/tools/user_management/__init__.py
+++ b/web/pgadmin/tools/user_management/__init__.py
@@ -383,6 +383,7 @@ def change_owner():
request.data, encoding='utf-8'
)
try:
+ new_user = User.query.get(data['new_owner'])
old_user_servers = Server.query.filter_by(shared=True, user_id=data[
'old_owner']).all()
server_group_ids = [server.servergroup_id for server in
@@ -400,12 +401,15 @@ def change_owner():
deleted_sg = []
# Change server user.
for server in old_user_servers:
+ sh_servers = SharedServer.query.filter_by(
+ servergroup_id=server.servergroup_id).all()
+
if old_sg_data[server.servergroup_id] in sg_data:
- sh_servers = SharedServer.query.filter_by(
- servergroup_id=server.servergroup_id).all()
+
for sh in sh_servers:
sh.servergroup_id = sg_data[
old_sg_data[server.servergroup_id]]
+ sh.server_owner = new_user.username
# Update Server user and server group to prevent deleting
# shared server associated with deleting user.
Server.query.filter_by(
@@ -422,6 +426,8 @@ def change_owner():
deleted_sg.append(server.servergroup_id)
else:
server.user_id = data['new_owner']
+ for sh in sh_servers:
+ sh.server_owner = new_user.username
# Change server group user.
for server_group in server_groups:
view thread (12+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [pgAdmin][RM-6143]: Shared server entries not getting deleted.
In-Reply-To: <CAOBg0ANTT=SeeG8yMaVUK_5UcO3fCGAae5KdzXAKL2GP3_kGeg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox