Thanks, patch applied. On Wed, Oct 21, 2020 at 4:45 PM Nikhil Mohite < nikhil.moh...@enterprisedb.com> 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 < > 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* >> > -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Sr. Software Architect* *EDB Postgres <http://edbpostgres.com>* *Mobile: +91 976-788-8246*
