Thanks, patch applied. On Mon, May 4, 2020 at 12:47 PM Satish V <satis...@enterprisedb.com> wrote:
> Hi Khushboo, > > As we discussed, the database disconnection error message is ignored as it > is getting displayed as expected. Variable name , info_already_connected, > is changed to already_connected. Run all the test cases except the feature > test. Number of test case failures before and after the patch remain the > same. > > Thanks, > Sathish V > > On Mon, May 4, 2020 at 12:04 AM Khushboo Vashi < > khushboo.va...@enterprisedb.com> wrote: > >> Hi Satish, >> >> On Thu, Apr 30, 2020 at 8:29 PM Satish V <satis...@enterprisedb.com> >> wrote: >> >>> Hi Khushboo, >>> >>> In the attached patch, >>> Fixed pep8 and made use of 'gettext' inside "database.js" file... >>> Regarding *res.info <http://res.info>='Database already connected', **we >>> are appending the database name and server name in front of 'res.info >>> <http://res.info>' string in the very next line. So using this 'Database >>> already connected' text directly inside alertify will stop it from showing >>> the info pertaining to the server and database. -* no changes are done >>> about res.info. >>> >>> The database disconnection error comes without a database name. To >> reproduce this issue, perform the steps in this RM and then try to >> disconnect the database. >> Also, please give the proper variable name as we discussed. >> >> Thanks, >> Khushboo >> >> Thanks >> >>> Sathish >>> >>> On Thu, Apr 30, 2020 at 7:40 AM Khushboo Vashi < >>> khushboo.va...@enterprisedb.com> wrote: >>> >>>> Hi Satish, >>>> >>>> Some minor comments: >>>> >>>> - Fix PEP8 issues. >>>> - Use gettext in database.js file for the info message. >>>> - No need to assign text value in res object (res.info = 'Database >>>> already connected.'), use this text directly in Alertify.info. >>>> >>>> Thanks, >>>> Khushboo >>>> >>>> >>>> >>>> >>>> >>>> On Thu, Apr 30, 2020 at 1:59 PM Satish V <satis...@enterprisedb.com> >>>> wrote: >>>> >>>>> Hi, >>>>> >>>>> I made the changes to the code such that conn.connect() will happen >>>>> only when there is no prior connection exist. In short, connection via >>>>> context menu will not trigger this conn.connect(). >>>>> In the client side code, instead of error alert, info alert is used to >>>>> inform the user. >>>>> >>>>> Kindly review the patch and suggest the changes if required. >>>>> >>>>> Thanks, >>>>> Sathish V >>>>> >>>>> On Thu, Apr 30, 2020 at 2:43 AM Khushboo Vashi < >>>>> khushboo.va...@enterprisedb.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Thu, Apr 30, 2020 at 12:04 PM Aditya Toshniwal < >>>>>> aditya.toshni...@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Thu, Apr 30, 2020 at 11:55 AM Khushboo Vashi < >>>>>>> khushboo.va...@enterprisedb.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Thu, Apr 30, 2020 at 10:55 AM Aditya Toshniwal < >>>>>>>> aditya.toshni...@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On Thu, Apr 30, 2020 at 10:48 AM Khushboo Vashi < >>>>>>>>> khushboo.va...@enterprisedb.com> wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>>> >>>>>>>>> I just checked the manager.connection function, and it is not >>>>>>>>> connecting or checking. It will just give the connection object >>>>>>>>> stored in >>>>>>>>> the memory. >>>>>>>>> >>>>>>>> :). >>>>>>>> >>>>>>>> >>>>>>>> databases/__init__.py : def connect() >>>>>>>> >>>>>>>> >>>>>>>> manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid) >>>>>>>> >>>>>>>> conn = manager.connection(did=did, auto_reconnect=True) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Above 2 lines will handle which I just just mentioned above. >>>>>>>> >>>>>>> I didn't find any code which will call the connect, apart >>>>>>> from auto_reconnect being used when we execute some query. >>>>>>> Nevertheless, I >>>>>>> still believe we should not remove the connect call for the sake of a >>>>>>> message, for which I think the user won't even bother. >>>>>>> >>>>>>>> >>>>>>>> >>>>>>> To understand the code properly, comment out the line conn.connect() >>>>>> from the def connect(), restart the pgadmin server and try to connect the >>>>>> database from the client side. >>>>>> So, you get to know that there is no harm in my suggestion. >>>>>> >>>>>>> 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. >>>>>>>>>> >>>>>>>>> Yes, info is a good option. >>>>>>>>> >>>>>>>>>> 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" >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Thanks and Regards, >>>>>>>>> Aditya Toshniwal >>>>>>>>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune >>>>>>>>> "Don't Complain about Heat, Plant a TREE" >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Thanks and Regards, >>>>>>> Aditya Toshniwal >>>>>>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune >>>>>>> "Don't Complain about Heat, Plant a TREE" >>>>>>> >>>>>> -- *Thanks & Regards* *Akshay Joshi* *Sr. Software Architect* *EnterpriseDB Software India Private Limited* *Mobile: +91 976-788-8246*
