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

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 <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> 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 <
> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>
>> 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 <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Thanks, patch applied.
>>>
>>> On Wed, Oct 7, 2020 at 12:11 PM Nikhil Mohite <
>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>
>>>> 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.
>>>>
>>>> On Tue, Oct 6, 2020 at 6:19 PM Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Nikhil
>>>>>
>>>>> Please verify and remove async = false wherever possible.
>>>>>
>>>>> On Tue, Oct 6, 2020 at 5:24 PM Dave Page <dave(dot)page(at)enterprisedb(dot)com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Oct 6, 2020 at 12:51 PM Murtuza Zabuawala <
>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>
>>>>>> I consider a sync ajax call to be a bug.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Regards,
>>>>>>> Murtuza Zabuawala
>>>>>>> *EDB*
>>>>>>> *POWER TO POSTGRES*
>>>>>>> https://www.edbpostgres.com
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Oct 1, 2020 at 1:31 PM Akshay Joshi <
>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Thanks, patch applied.
>>>>>>>>
>>>>>>>> On Thu, Oct 1, 2020 at 10:42 AM Nikhil Mohite <
>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> 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 <
>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> 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 <
>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>
>>>>>>>>>>> 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 <
>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> 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 <
>>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Sep 17, 2020 at 4:15 PM Nikhil Mohite <
>>>>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 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.*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>>>> *pgAdmin Hacker | Sr. Software Architect*
>>>>>>>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>> *pgAdmin Hacker | Sr. Software Architect*
>>>>>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>>>>>
>>>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>> *pgAdmin Hacker | Sr. Software Architect*
>>>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>>>
>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> *Thanks & Regards*
>>>>>>>> *Akshay Joshi*
>>>>>>>> *pgAdmin Hacker | Sr. Software Architect*
>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>
>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> VP & Chief Architect, Database Infrastructure
>>>>>> EDB: http://www.enterprisedb.com
>>>>>>
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> *Thanks & Regards*
>>>>> *Akshay Joshi*
>>>>> *pgAdmin Hacker | Sr. Software Architect*
>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>
>>>>> *Mobile: +91 976-788-8246*
>>>>>
>>>>
>>>
>>> --
>>> *Thanks & Regards*
>>> *Akshay Joshi*
>>> *pgAdmin Hacker | Sr. Software Architect*
>>> *EDB Postgres <http://edbpostgres.com>*
>>>
>>> *Mobile: +91 976-788-8246*
>>>
>>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
> *pgAdmin Hacker | Sr. Software Architect*
> *EDB Postgres <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>