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"
>>>
>>
