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"
>>>>>>
>>>>>
diff --git a/web/pgadmin/browser/server_groups/servers/databases/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/__init__.py
index 9216f21..9e09419 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/__init__.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/__init__.py
@@ -456,30 +456,33 @@ class DatabaseView(PGChildNodeView):
from pgadmin.utils.driver import get_driver
manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid)
conn = manager.connection(did=did, auto_reconnect=True)
- status, errmsg = conn.connect()
-
- if not status:
- current_app.logger.error(
- "Could not connected to database(#{0}).\nError: {1}".format(
- did, errmsg
+ already_connected = conn.connected()
+ if not already_connected:
+ status, errmsg = conn.connect()
+ if not status:
+ current_app.logger.error(
+ "Could not connected to database(#{0}).\nError: {1}"
+ .format(
+ did, errmsg
+ )
)
- )
-
- return internal_server_error(errmsg)
- else:
- current_app.logger.info('Connection Established for Database Id: \
- %s' % (did))
-
- return make_json_response(
- success=1,
- info=_("Database connected."),
- data={
- 'icon': 'pg-icon-database',
- 'connected': True,
- 'info_prefix': '{0}/{1}'.
- format(Server.query.filter_by(id=sid)[0].name, conn.db)
- }
- )
+ return internal_server_error(errmsg)
+ else:
+ current_app.logger.info(
+ 'Connection Established for Database Id: \
+ %s' % (did)
+ )
+ return make_json_response(
+ success=1,
+ info=_("Database connected."),
+ data={
+ 'icon': 'pg-icon-database',
+ 'already_connected': already_connected,
+ 'connected': True,
+ 'info_prefix': '{0}/{1}'.
+ format(Server.query.filter_by(id=sid)[0].name, conn.db)
+ }
+ )
def disconnect(self, gid, sid, did):
"""Disconnect the database."""
diff --git a/web/pgadmin/browser/server_groups/servers/databases/static/js/database.js b/web/pgadmin/browser/server_groups/servers/databases/static/js/database.js
index f458d69..af2de4a 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/static/js/database.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/static/js/database.js
@@ -520,7 +520,6 @@ define('pgadmin.node.database', [
tree.deselect(item);
tree.setInode(item);
}
-
if (res && res.data) {
if(typeof res.data.connected == 'boolean') {
data.connected = res.data.connected;
@@ -530,11 +529,17 @@ define('pgadmin.node.database', [
data.icon = res.data.icon;
tree.addIcon(item, {icon: data.icon});
}
+ if(res.data.already_connected) {
+ res.info = gettext('Database already connected.');
+ }
if(res.data.info_prefix) {
res.info = `${_.escape(res.data.info_prefix)} - ${res.info}`;
}
-
- Alertify.success(res.info);
+ if(res.data.already_connected) {
+ Alertify.info(res.info);
+ } else {
+ Alertify.success(res.info);
+ }
obj.trigger('connected', obj, item, data);
pgBrowser.Events.trigger(
'pgadmin:database:connected', item, data
