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.
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..c8dba00 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
+ info_already_connected = conn.connected()
+ if not info_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',
+ 'info_already_connected': info_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..1dd25f2 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.info_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.info_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
