On Thu, Apr 30, 2020 at 10:14 AM Aditya Toshniwal < aditya.toshni...@enterprisedb.com> wrote:
> Hi, > > > On Thu, Apr 30, 2020 at 9:41 AM Satish V <satis...@enterprisedb.com> > wrote: > >> Hi Kushboo, >> >> Thanks for the update. I will check the same and make appropriate changes. >> >> Thanks, >> Sathish >> >> On Thu, Apr 30, 2020 at 9:20 AM Khushboo Vashi < >> khushboo.va...@enterprisedb.com> wrote: >> >>> Hi Satish, >>> >>> As per the RM, the fix is supposed to be at the front-end but it seems >>> difficult at the moment as on the selection of the database, we connect it >>> and at the same time the context menu is being called. >>> As you have tried to fix at the backend, some of the review comments are >>> below. >>> >>> 1. If the database is already connected, no need to call conn.connect >>> again. >>> >>> >>> info_already_connected = conn.connected() >>> >>> status, errmsg = conn.connect() >>> >>> I've noticed conn.connected() is misleading sometimes. Let's say if the > PG server is stopped and no query is fired from pgadmin after that, then > conn.connected() > will still give True. It is updated only when a query is fired to the PG > server. I would suggest let it connect again as there is no harm and this > function is very important. We don't want to mess it up for the sake of a > message. > It's true that conn.connected() is misleading but we already get the connection before checking conn.connected() with conn = manager.connection( did=did, auto_reconnect=True). So, if the database server is stopped, it will throw the error. > 2. If you want to raise an error at the client side, use the appropriate > function or status (check file ajax.py) at the server side and send the > appropriate response. >>> >>> Below code needs to be changed. >>> >>> if(res.data.info_already_connected){ >>> >>> Alertify.error(res.info); >>> >>> } else { >>> >>> Alertify.success(res.info); >>> >>> } >>> >>> The problem is test cases. Almost all the test cases have checked for > response "Database connected.". If the function is changed at the server > side then all the test cases (around 85+) would have to be changed to > handle this. > It's a valid point, so I would suggest to show the warning/info at the client side, no need to show the error message. > Thanks, >>> >>> Khushboo >>> >>> >>> >>> On Wed, Apr 29, 2020 at 8:20 PM Satish V <satis...@enterprisedb.com> >>> wrote: >>> >>>> Hi Hackers, >>>> >>>> In the patch attached, we are gracefully informing the end user, using >>>> an alert message, that the database is already connected when they click >>>> "Connect Database..." after right clicking on a disconnected database. >>>> >>>> As this problem deals with racing conditions, it is highly complex to >>>> show the "Disconnect database" option in the menu upon right click. So we >>>> are alerting the end user with the information of "Database already >>>> connected". >>>> >>>> Thanks, >>>> Sathish V >>>> >>> > > -- > Thanks and Regards, > Aditya Toshniwal > pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune > "Don't Complain about Heat, Plant a TREE" >