Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kVAMq-0003WR-Co for pgadmin-hackers@arkaria.postgresql.org; Wed, 21 Oct 2020 09:25:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kVAMo-0001eF-Gj for pgadmin-hackers@arkaria.postgresql.org; Wed, 21 Oct 2020 09:25:10 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kVAMo-0001e7-4Q for pgadmin-hackers@lists.postgresql.org; Wed, 21 Oct 2020 09:25:10 +0000 Received: from mail-io1-xd36.google.com ([2607:f8b0:4864:20::d36]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kVAMk-0004kk-17 for pgadmin-hackers@postgresql.org; Wed, 21 Oct 2020 09:25:09 +0000 Received: by mail-io1-xd36.google.com with SMTP id b8so2272604ioh.11 for ; Wed, 21 Oct 2020 02:25:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mOioEp7Uv1Y27DIuJ3Di/BhL6h545NE9XKlChsRfkU4=; b=sP6ldgj6I1OC6eWAzr7U8Ri82VOhYTi+da5HDf0nOumQPwI/oq0/CQIPTenRHoAKKd iOYCLKl+KXgC8zDwXZtPtf/JNhQZvUhTX0Lls7bGvH32VvY5SoJDSGTkup5wx7TUa5Os ak6B5d9qVn1rdMVMRKKhrXmQoXbmMwv0dKTh/S32qrzuO4NvZvjdTnV1CJQ1l2QIXzg2 J31yHfAM8SKt2hMJfINCAIdJMJ4nQOL4xIUNBa+im5R9wkMc5C1Wm8naOBXPdgsLtJgb JlCL9fHDyRSHlmYndH8KJkPEEDAY0YIKf0BdDUfUhoBIBpYeohZxMczIBWkLVj+gm7pw WxPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mOioEp7Uv1Y27DIuJ3Di/BhL6h545NE9XKlChsRfkU4=; b=PN9vFdBrEixYHXxWRgVssjhtZUuE57NismEHdf+oQCVkozjo48NqOC6uuDb4ZYqzRb EHBMDkvUAaghnrH8ioOYCS/VFBNRB3tusp8r2le0J3CI5eXBBbHZ5ggSuaz9o+kXNzRX EHiG3bCGeA0Ygwx5stWikFq7oij5CEMhjIZRRz3xgfrfzqyCj5torK8aY0pePgxpQFbH 5Y3P082KWidfY5x8DWLC3J2ilkKDdz+FDSImTuNeMVbarFuIQlOf4/rCMsLWFlVlnxzj wWQcVmGCCJJt3SuOPzdqKYOHXN5CAXpd7+ZugnnLXI7zImXksGebeX8hINA5eko91D7S IRDw== X-Gm-Message-State: AOAM530kFvq7vRVzmkAfoxTJwyT1a2YQSqXh+NCJ1kltu+peNgpWzw4v mLkdhfdGQ2w325UuPmH1/8S+jeLSE8Z49Fayyf19cQKw8tM0sCNW3efKwspyI3C/3S3B+NfP020 7FY7yl9MT0gxESepfq0sjK6NMm+Bdm8bfRXxMEBEf9Kz+BTtOAG0oSVsscXH+YqH0CyD0LhRxLr XShymlQnJiulfJEn80xb01Nh768q3r/wx+3NV9AI0yFQLfhiBanneQ9g+qGQ== X-Google-Smtp-Source: ABdhPJwDQkXqyQPUcVEasXGxmfjp3bPCIpR0LeaT8j+UNjFaiZywS4V3ROEaRkl9apYRVt88vlH6q3OJnH9EskzLoTs= X-Received: by 2002:a02:7125:: with SMTP id n37mr749845jac.1.1603272303793; Wed, 21 Oct 2020 02:25:03 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Wed, 21 Oct 2020 14:54:52 +0530 Message-ID: Subject: Re: [pgAdmin][RM3794]:Allow User to Change Database Connection from an Open Query Tool Tab To: Nikhil Mohite Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000025777f05b22aeccf" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000025777f05b22aeccf Content-Type: text/plain; charset="UTF-8" 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.mohite@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.joshi@enterprisedb.com> wrote: > >> Thanks, patch applied. >> >> On Wed, Oct 7, 2020 at 12:11 PM Nikhil Mohite < >> nikhil.mohite@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.joshi@enterprisedb.com> wrote: >>> >>>> Hi Nikhil >>>> >>>> Please verify and remove async = false wherever possible. >>>> >>>> On Tue, Oct 6, 2020 at 5:24 PM Dave Page >>>> wrote: >>>> >>>>> >>>>> >>>>> On Tue, Oct 6, 2020 at 12:51 PM Murtuza Zabuawala < >>>>> murtuza.zabuawala@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.joshi@enterprisedb.com> wrote: >>>>>> >>>>>>> Thanks, patch applied. >>>>>>> >>>>>>> On Thu, Oct 1, 2020 at 10:42 AM Nikhil Mohite < >>>>>>> nikhil.mohite@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.joshi@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.mohite@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.joshi@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.mohite@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.joshi@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.mohite@enterprisedb.com> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Team, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regarding RM-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* >>>>>>>>>>>>>> *Mob.No: +91-7798364578.* >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> *Thanks & Regards* >>>>>>>>>>>>> *Akshay Joshi* >>>>>>>>>>>>> *pgAdmin Hacker | Sr. Software Architect* >>>>>>>>>>>>> *EDB Postgres * >>>>>>>>>>>>> >>>>>>>>>>>>> *Mobile: +91 976-788-8246* >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> *Thanks & Regards* >>>>>>>>>>> *Akshay Joshi* >>>>>>>>>>> *pgAdmin Hacker | Sr. Software Architect* >>>>>>>>>>> *EDB Postgres * >>>>>>>>>>> >>>>>>>>>>> *Mobile: +91 976-788-8246* >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> *Thanks & Regards* >>>>>>>>> *Akshay Joshi* >>>>>>>>> *pgAdmin Hacker | Sr. Software Architect* >>>>>>>>> *EDB Postgres * >>>>>>>>> >>>>>>>>> *Mobile: +91 976-788-8246* >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> *Thanks & Regards* >>>>>>> *Akshay Joshi* >>>>>>> *pgAdmin Hacker | Sr. Software Architect* >>>>>>> *EDB Postgres * >>>>>>> >>>>>>> *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 * >>>> >>>> *Mobile: +91 976-788-8246* >>>> >>> >> >> -- >> *Thanks & Regards* >> *Akshay Joshi* >> *pgAdmin Hacker | Sr. Software Architect* >> *EDB Postgres * >> >> *Mobile: +91 976-788-8246* >> > -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Sr. Software Architect* *EDB Postgres * *Mobile: +91 976-788-8246* --00000000000025777f05b22aeccf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Nikhil

Following are the review comm= ents:
  • Connect to any server from the browser tree. Open t= he query tool and then open=C2=A0the new connection dialog. Click on the &q= uot;OK" button without changing=C2=A0any field. It shows the popup for= "Change connection" which should not be raised because=C2=A0the = server is the same.
  • In the above scenario, if you click on the Yes = button it is showing a=C2=A0duplicate=C2=A0entry for the same server.
    • <= li>The server name is not getting changed when we connect to any other=C2= =A0server from the new connection. Changes needed in alertify message, tab = title, and a combo box.
  • Remove the "." from the "Cha= nge connection." title.
  • Change the string "Change connect= ion will lose all non committed changes for current connection, do you want= to continue?"=C2=A0 to "By changing the connection you will l= ose all your unsaved data for the current connection.
    Do you wan= t to continue?"
Please fix the above changes and se= nd the patch again.

On Wed, Oct 21, 2020 at 11:08 AM Nikhil Mohite <= ;nikhil.mohite@enterprise= db.com> wrote:
Hi Akshay,

I have updated the exi= sting=C2=A0implementation 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. No= tification to the user before the change connection action.
4. If= we connect to the server through a new connection dialog, the=C2=A0tree wi= ll use the same connection and it will not create a new connection.
(In earlier implementation it was asking for the password even we=C2=A0h= ave connected from a new connection dialog.)

PFA p= atch

Regards,
Nikhil Mohite.
=
On Thu= , Oct 8, 2020 at 11:39 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wr= ote:
Thanks, patch applied.

On Wed, Oct 7, 2020 at 12:11 PM Nikhil Mohite= <ni= khil.mohite@enterprisedb.com> wrote:
Hi Akshay,

<= div>I checked the implementation and found 2 locations which I missed in th= e last patch to remove async: False.
I have removed all occurrenc= es of async: False now also=C2=A0added missing loader in required places.

PFA updated the patch for the same.

<= /div>
Regards,
Nikhil Mohite.

On Tue, Oct 6, 2020 at 6:1= 9 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Nikhil=C2=A0=

Please verify and remove async =3D false wherever possi= ble.

On Tue, Oct 6, 2020 at 5:24 PM Dave Page <dave.page@enterprisedb.com&= gt; wrote:


On Tue, Oct 6, 2020 at 12:51 PM Murtuza Za= buawala <murtuza.zabuawala@enterprisedb.com> wrote:
Hi Akshay,

We have used ays= nc=3DFalse in most ajax calls with this feature, It is causing=C2=A0UI 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 c= alls as much as possible.

I con= sider a sync ajax call to be a bug.
=C2=A0


<= div dir=3D"ltr">
--
Regards,
Murtuza Zabuawala=
EDB<= /b>
POWER TO POSTG= RES


On Thu, Oct 1, 2020 at 1:31 PM Akshay Joshi <akshay.joshi@enterprisedb.com= > wrote:
=
Thanks, patch applied.

On Thu, Oct 1, 2020 at 10:42 AM = Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:
Hi Akshay,

I have=C2=A0resolved the sonarQube issues, PFA updated pa= tch for the same.


Regards,
Nikhil Mohite.


<= div dir=3D"ltr" class=3D"gmail_attr">On Tue, Sep 29, 2020 at 11:31 AM Aksha= y Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi=C2=A0Nikhil

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 Mo= hite <nikhil.mohite@enterprisedb.com> wrote:
Hi Akshay,

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

Regards,
Nikhil Mo= hite.

On Mon, Sep 28, 2020 at 5:58 PM Akshay Joshi <akshay.joshi@enterprise= db.com> wrote:
Hi=C2=A0Nikhil

The patch is not a= pplying, rebase, and send it again. Please check your code should not creat= e any new SonarQube issues.

On Mon, Sep 28, 2020 at 11:20 AM Nikhil Mo= hite <nikhil.mohite@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 p= atch.



On Mon, Sep 21, 2020 at 5:24 PM Ak= shay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi=C2=A0Nikhil
Following are the initial review comments:
  • Open Vie= w/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 position= al argument but 2 were given" error.
  • In my opinion, we should = hide the option to change the database connection for View/Edit Data.
    • <= li>If the user clicks on the same database connection multiple times then n= o 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 Passwor= d field should not be there on the "connect to server" dialog. So= metimes we saved the password so asking a password every time is not correc= t. Check the pgAdmin 3 behavior.
Code review still remains.
<= /div>

On Thu, Sep 17, 2020 at 4:15 PM Nikhil Mohite <nikhil.mohite@enterprisedb.= com> wrote:
Hi Team,

Regarding RM-3794 allow t= he=C2=A0user to change the database connection from an open query tool:
I have implemented the feature and also added documentation for it.<= /div>

PFA patch.

--
Thanks &= ; Regards,
Nikhil Mohi= te
Software Engi= neer.


--


--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Sr. Software Ar= chitect
EDB Postgres=
Mobile: +91 976-788-8246



--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Sr. Software Ar= chitect
EDB Postgres=
Mobile: +91 976-788-8246



--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Sr. Software Ar= chitect
EDB Postgres=
Mobile: +91 976-788-8246



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

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


--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Sr. Software Ar= chitect
EDB Postgres=
Mobile: +91 976-788-8246



--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Sr. Software Ar= chitect
EDB Postgres=
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmi= n Hacker | Sr. Software Architect
EDB Postgres
Mobile: +91 976-788-8246
=
--00000000000025777f05b22aeccf--