Subject: [pgAdmin][RM3794]:Allow User to Change Database Connection from an Open Query Tool Tab

From: Nikhil Mohite <[email-masked]>
Date: Thu, 17 Sep 2020 16:15:03 +0530

Hi Team,

Regarding RM-3794 <https://redmine.postgresql.org/issues/3794> allow
the user to change the database connection from an open query tool:
I have implemented the feature and also added documentation for it.

PFA patch.

-- 
*Thanks & Regards,*
*Nikhil Mohite*
*Software Engineer.*
*EDB Postgres* <https://www.enterprisedb.com/>
*Mob.No: +91-7798364578.*

---

From: Akshay Joshi <[email-masked]>
Date: Mon, 21 Sep 2020 17:24:06 +0530

Hi Nikhil

Following are the initial review comments:

   - Open View/Edit data on any table and click on the same database
   connection and then click on the Execute button. Got "get_primary_keys()
   takes 1 positional argument but 2 were given" error.
   - In my opinion, we should hide the option to change the database
   connection for View/Edit Data.
   - If the user clicks on the same database connection multiple times then
   no need to change the backend connection and transaction id. Add validation
   at the backend, no action required in this case.
   - The role option is missing from the "connect to server" dialog.
   - The Password field should not be there on the "connect to server"
   dialog. Sometimes we saved the password so asking a password every time is
   not correct. Check the pgAdmin 3 behavior.

Code review still remains.

-- 
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Sr. Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*

---

From: Nikhil Mohite <[email-masked]>
Date: Mon, 28 Sep 2020 11:20:11 +0530

Hi Akshay,

I have resolved all the review comments and also updated the test cases as
per the new implementation.

PFA updated patch.

On Mon, Sep 21, 2020 at 5:24 PM Akshay Joshi <[email-masked]>
wrote:

>    - If the user clicks on the same database connection multiple times
>    then no need to change the backend connection and transaction id. Add
>    validation at the backend, no action required in this case.

---

From: Akshay Joshi <[email-masked]>
Date: Mon, 28 Sep 2020 17:58:42 +0530

Hi Nikhil

The patch is not applying, rebase, and send it again. Please check your
code should not create any new SonarQube issues.

On Mon, Sep 28, 2020 at 11:20 AM Nikhil Mohite <
[email-masked]> wrote:

> On Mon, Sep 21, 2020 at 5:24 PM Akshay Joshi <
> [email-masked]> wrote:
>
>> Hi Nikhil
>>

---

From: Nikhil Mohite <[email-masked]>
Date: Mon, 28 Sep 2020 19:31:10 +0530

Hi Akshay,

I have resolved code conflict issues and sonarqube issues.
PFA updated patch.

Regards,
Nikhil Mohite.

On Mon, Sep 28, 2020 at 5:58 PM Akshay Joshi <[email-masked]>
wrote:

>> I have resolved all the review comments and also updated the test cases
>> as per the new implementation.
>>
>> PFA updated patch.
>>
>>
>>

---

From: Akshay Joshi <[email-masked]>
Date: Tue, 29 Sep 2020 11:31:37 +0530

Hi Nikhil

Your patch introduces 1 new Bug and 13 new code smells, please fix those
and resend the patch.

On Mon, Sep 28, 2020 at 7:31 PM Nikhil Mohite <
[email-masked]> wrote:

> On Mon, Sep 28, 2020 at 5:58 PM Akshay Joshi <

---

From: Nikhil Mohite <[email-masked]>
Date: Thu, 1 Oct 2020 10:42:23 +0530

Hi Akshay,

I have resolved the sonarQube issues, PFA updated patch for the same.

Regards,
Nikhil Mohite.

On Tue, Sep 29, 2020 at 11:31 AM Akshay Joshi <[email-masked]>
wrote:

>>>>>    - If the user clicks on the same database connection multiple
>>>>>    times then no need to change the backend connection and transaction id. Add
>>>>>    - The Password field should not be there on the "connect to
>>>>>    server" dialog. Sometimes we saved the password so asking a password every
>>>>>    time is not correct. Check the pgAdmin 3 behavior.
>>>>>

---

From: Akshay Joshi <[email-masked]>
Date: Thu, 1 Oct 2020 13:31:31 +0530

Thanks, patch applied.

On Thu, Oct 1, 2020 at 10:42 AM Nikhil Mohite <
[email-masked]> wrote:

> On Tue, Sep 29, 2020 at 11:31 AM Akshay Joshi <
>>>>> I have resolved all the review comments and also updated the test
>>>>> cases as per the new implementation.
>>>>>
>>>>> PFA updated patch.
>>>>>
>>>>>
>>>>>
>>>>>>> Regarding RM-3794 <https://redmine.postgresql.org/issues/3794>
>>>>>>> allow the user to change the database connection from an open query tool:

---

From: Nikhil Mohite <[email-masked]>
Date: Tue, 6 Oct 2020 14:21:47 +0530

Hi Akshay,

I have updated the following points as per suggestions:
1. Updated connection success message. (Added database name in the success
message.)
2. Resolve issue of the mouse pointer and dropdown will show below the
connection string only.
3. Added loader for both new connections and load existing connections.
4. Removed async: false for update connection.

Regards,
Nikhil Mohite.

On Thu, Oct 1, 2020 at 1:31 PM Akshay Joshi <[email-masked]>
wrote:

>>>>> The patch is not applying, rebase, and send it again. Please check
>>>>> your code should not create any new SonarQube issues.
>>>>>
>>>>>>>    - Open View/Edit data on any table and click on the same
>>>>>>>    database connection and then click on the Execute button. Got
>>>>>>>    "get_primary_keys() takes 1 positional argument but 2 were given" error.
>>>>>>>    - In my opinion, we should hide the option to change the
>>>>>>>    database connection for View/Edit Data.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Tue, 6 Oct 2020 17:21:17 +0530

Hi Akshay,

We have used aysnc=False in most ajax calls with this feature, It is
causing UI hang in case of slow server response.
You can try adding a time.sleep() call at the python side response and
check the UI hang, I think we should avoid sync calls as much as possible.

--
Regards,
Murtuza Zabuawala
*EDB*
*POWER TO POSTGRES*
https://www.edbpostgres.com

---

From: Dave Page <[email-masked]>
Date: Tue, 6 Oct 2020 12:54:09 +0100

> check the UI hang, I think we should avoid sync calls as much as possible.

I consider a sync ajax call to be a bug.

>>>> Your patch introduces 1 new Bug and 13 new code smells, please fix
>>>> those and resend the patch.
>>>>

-- 
Dave Page
VP & Chief Architect, Database Infrastructure
EDB: http://www.enterprisedb.com

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

---

From: Akshay Joshi <[email-masked]>
Date: Tue, 6 Oct 2020 18:19:17 +0530

Hi Nikhil

Please verify and remove async = false wherever possible.

On Tue, Oct 6, 2020 at 5:24 PM Dave Page <[email-masked]> wrote:

>> On Thu, Oct 1, 2020 at 1:31 PM Akshay Joshi <

---

From: Nikhil Mohite <[email-masked]>
Date: Wed, 7 Oct 2020 12:11:41 +0530

Hi Akshay,

I checked the implementation and found 2 locations which I missed in the
last patch to remove async: False.
I have removed all occurrences of async: False now also added missing
loader in required places.

PFA updated the patch for the same.

Regards,
Nikhil Mohite.

---

From: Akshay Joshi <[email-masked]>
Date: Thu, 8 Oct 2020 11:39:17 +0530

Thanks, patch applied.

On Wed, Oct 7, 2020 at 12:11 PM Nikhil Mohite <
[email-masked]> wrote:

>>>>>>>>>>>    - If the user clicks on the same database connection
>>>>>>>>>>>    multiple times then no need to change the backend connection and
>>>>>>>>>>>    transaction id. Add validation at the backend, no action required in this
>>>>>>>>>>>    case.

---

From: Nikhil Mohite <[email-masked]>
Date: Wed, 21 Oct 2020 11:08:46 +0530

Hi Akshay,

I have updated the existing implementation as per suggestions.
1. Show servers in server groups in the dropdown.
2. Current selected connection in the new connection dropdown is now
highlighted as selected.
3. Notification to the user before the change connection action.
4. If we connect to the server through a new connection dialog, the tree
will use the same connection and it will not create a new connection.
(In earlier implementation it was asking for the password even we have
connected from a new connection dialog.)

PFA patch

Regards,
Nikhil Mohite.

On Thu, Oct 8, 2020 at 11:39 AM Akshay Joshi <[email-masked]>
wrote:

>> On Tue, Oct 6, 2020 at 6:19 PM Akshay Joshi <
>>>>>>>>>> The patch is not applying, rebase, and send it again. Please
>>>>>>>>>> check your code should not create any new SonarQube issues.
>>>>>>>>>>
>>>>>>>>>>> I have resolved all the review comments and also updated the
>>>>>>>>>>> test cases as per the new implementation.
>>>>>>>>>>>
>>>>>>>>>>> PFA updated patch.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>

---

From: Akshay Joshi <[email-masked]>
Date: Wed, 21 Oct 2020 14:54:52 +0530

Hi Nikhil

Following are the review comments:

   - Connect to any server from the browser tree. Open the query tool and
   then open the new connection dialog. Click on the "OK" button without
   changing any field. It shows the popup for "Change connection" which should
   not be raised because the server is the same.
   - In the above scenario, if you click on the Yes button it is showing
   a duplicate entry for the same server.
   - The server name is not getting changed when we connect to any
   other server from the new connection. Changes needed in alertify message,
   tab title, and a combo box.
   - Remove the "." from the "Change connection." title.
   - Change the string "Change connection will lose all non committed
   changes for current connection, do you want to continue?"  to "*By
   changing the connection you will lose all your unsaved data for the current
   connection.*
   *Do you want to continue?*"

Please fix the above changes and send the patch again.

On Wed, Oct 21, 2020 at 11:08 AM Nikhil Mohite <
[email-masked]> wrote:

> On Thu, Oct 8, 2020 at 11:39 AM Akshay Joshi <
>>>>>> You can try adding a time.sleep() call at the python side response
>>>>>> and check the UI hang, I think we should avoid sync calls as much as
>>>>>> possible.
>>>>>>
>>>>>
>>>>>>>>>>>>>    - The Password field should not be there on the "connect
>>>>>>>>>>>>>    to server" dialog. Sometimes we saved the password so asking a password
>>>>>>>>>>>>>    every time is not correct. Check the pgAdmin 3 behavior.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Regarding RM-3794
>>>>>>>>>>>>>> <https://redmine.postgresql.org/issues/3794> allow the user
>>>>>>>>>>>>>> to change the database connection from an open query tool:

---

From: Nikhil Mohite <[email-masked]>
Date: Wed, 21 Oct 2020 16:45:13 +0530

Hi Akshay,

I have fixed the review comments, PFA the updated patch for the same.

Regards,
Nikhil Mohite.

On Wed, Oct 21, 2020 at 2:55 PM Akshay Joshi <[email-masked]>
wrote:

>>>> I checked the implementation and found 2 locations which I missed in
>>>> the last patch to remove async: False.
>>>>>>>>>> Your patch introduces 1 new Bug and 13 new code smells, please
>>>>>>>>>> fix those and resend the patch.
>>>>>>>>>>

---

From: Akshay Joshi <[email-masked]>
Date: Wed, 21 Oct 2020 17:19:39 +0530

Thanks, patch applied.

On Wed, Oct 21, 2020 at 4:45 PM Nikhil Mohite <
[email-masked]> wrote:

> On Wed, Oct 21, 2020 at 2:55 PM Akshay Joshi <
>>    - Connect to any server from the browser tree. Open the query tool
>>    and then open the new connection dialog. Click on the "OK" button without
>>>>>>>> We have used aysnc=False in most ajax calls with this feature, It
>>>>>>>> is causing UI hang in case of slow server response.
>>>>>>>>>>>>>>>> <https://redmine.postgresql.org/issues/3794> allow