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.jo...@enterprisedb.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.moh...@enterprisedb.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.jo...@enterprisedb.com> wrote: >> >>> Thanks, patch applied. >>> >>> On Wed, Oct 7, 2020 at 12:11 PM Nikhil Mohite < >>> nikhil.moh...@enterprisedb.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.jo...@enterprisedb.com> wrote: >>>> >>>>> Hi Nikhil >>>>> >>>>> Please verify and remove async = false wherever possible. >>>>> >>>>> On Tue, Oct 6, 2020 at 5:24 PM Dave Page <dave.p...@enterprisedb.com> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Tue, Oct 6, 2020 at 12:51 PM Murtuza Zabuawala < >>>>>> murtuza.zabuaw...@enterprisedb.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.jo...@enterprisedb.com> wrote: >>>>>>> >>>>>>>> Thanks, patch applied. >>>>>>>> >>>>>>>> On Thu, Oct 1, 2020 at 10:42 AM Nikhil Mohite < >>>>>>>> nikhil.moh...@enterprisedb.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.jo...@enterprisedb.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.moh...@enterprisedb.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.jo...@enterprisedb.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.moh...@enterprisedb.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.jo...@enterprisedb.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.moh...@enterprisedb.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* >
RM_3794_review_comments_v2.patch
Description: Binary data
