public inbox for [email protected]
help / color / mirror / Atom feedFrom: Akshay Joshi <[email protected]>
To: Nikhil Mohite <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin][RM3794]:Allow User to Change Database Connection from an Open Query Tool Tab
Date: Wed, 21 Oct 2020 17:19:39 +0530
Message-ID: <CANxoLDeGKhoGD+L_=zK=gvZm9STT1PGYHyK6JfrCaK+=WLj7uw@mail.gmail.com> (raw)
In-Reply-To: <CAOBg0AMeE1=Mv17m=AjSnkB-GzXiQvtOfH4zttTAVbiyNJ=45A@mail.gmail.com>
References: <CAOBg0AM=b7hfJjTJAXu=wGkKHEsV_zoZpSO-QU6Z-G9p2wVF0w@mail.gmail.com>
<CANxoLDdUj_RKgQWFE6F7noEJbDz+P_3-njnn2mf10TSa0V9wdw@mail.gmail.com>
<CAOBg0AOHJT4wUC3AXjHV2aorF8_OpP3erah8yhyi2eKwn6F_Sg@mail.gmail.com>
<CANxoLDca1AtBKrnW8rshWqsd_iwHEfXV2PeqCnUQnoEonfs52g@mail.gmail.com>
<CAOBg0APx0+twWJah7RaE9ZmDsBXUnbagikR7NyG3fUD21GYd8Q@mail.gmail.com>
<CANxoLDdkvhUE3pohPPgyyz2tYwYej+VAkxUE-e09JBxd3Aeh0w@mail.gmail.com>
<CAOBg0ANsr6eE_4+6ApKYhj5m5N+ChbQgj_yWgufh0KqiZrovsw@mail.gmail.com>
<CANxoLDfn8cm8YiK8e1rUWRZ+jU7AAr1krdwQb-giDg-B3ATzsQ@mail.gmail.com>
<CAKKotZRSjJTKzcappNGZ0yJiuiwxBfBxoex+6fXF3RzdbEKDBg@mail.gmail.com>
<CA+OCxoyfycZAT5Wc6L72HCXKrPTgkj9jcR4FgO6TKZN5KvZ_Ow@mail.gmail.com>
<CANxoLDcZYnhT0xw5SLs8VCS60LPFLW_OU+NEzcNs747uhv_a=A@mail.gmail.com>
<CAOBg0AMU_yTSrVU6Z+Q0Je4PuE3ncALdDA3Q=QUjQeXiWuHS6w@mail.gmail.com>
<CANxoLDd1M0kY4jM5Me=cUpejFt5cAQV9Uz8Sv6i7C3LgYJOLJg@mail.gmail.com>
<CAOBg0AOnEOhFV-8-M2J=PRk3KUta8nagLt-RB3pQO2JG3EEckg@mail.gmail.com>
<CANxoLDfa1oHTdieuGjAU9OsdTzeZhh7Uz4dWQ9fjzUkJHhuGrg@mail.gmail.com>
<CAOBg0AMeE1=Mv17m=AjSnkB-GzXiQvtOfH4zttTAVbiyNJ=45A@mail.gmail.com>
Thanks, patch applied.
On Wed, Oct 21, 2020 at 4:45 PM Nikhil Mohite <
[email protected]> wrote:
> 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 protected]> 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 <
>> [email protected]> 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 <
>>> [email protected]> wrote:
>>>
>>>> Thanks, patch applied.
>>>>
>>>> On Wed, Oct 7, 2020 at 12:11 PM Nikhil Mohite <
>>>> [email protected]> 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 <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hi Nikhil
>>>>>>
>>>>>> Please verify and remove async = false wherever possible.
>>>>>>
>>>>>> On Tue, Oct 6, 2020 at 5:24 PM Dave Page <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Oct 6, 2020 at 12:51 PM Murtuza Zabuawala <
>>>>>>> [email protected]> 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 <
>>>>>>>> [email protected]> wrote:
>>>>>>>>
>>>>>>>>> Thanks, patch applied.
>>>>>>>>>
>>>>>>>>> On Thu, Oct 1, 2020 at 10:42 AM Nikhil Mohite <
>>>>>>>>> [email protected]> 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 <
>>>>>>>>>> [email protected]> 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 <
>>>>>>>>>>> [email protected]> 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 <
>>>>>>>>>>>> [email protected]> 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 <
>>>>>>>>>>>>> [email protected]> 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 <
>>>>>>>>>>>>>> [email protected]> 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 <
>>>>>>>>>>>>>>> [email protected]> 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*
>>
>
--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Sr. Software Architect*
*EDB Postgres <http://edbpostgres.com>*
*Mobile: +91 976-788-8246*
view thread (18+ messages)
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][RM3794]:Allow User to Change Database Connection from an Open Query Tool Tab
In-Reply-To: <CANxoLDeGKhoGD+L_=zK=gvZm9STT1PGYHyK6JfrCaK+=WLj7uw@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