Re: Query Tool JS Minimal Refactoring

Yosry Muhammad <[email protected]>
Newsgroups gmane.comp.db.postgresql.pgadmin.devel
Message-ID <CAFSMqn_QkJh8X8SsO2RpWV=f5bC4faYegBBbC9kh+iVOJ1ACyg@mail.gmail.com>
Please find an updated patch attached.

On Mon, Aug 19, 2019 at 9:54 AM Yosry Muhammad <[email protected]> wrote:

> Jasmine tests passed on my machine, I will take another look once I have
> access to my machine.
>
> On Mon, Aug 19, 2019, 7:57 AM Akshay Joshi <[email protected]>
> wrote:
>
>> Hi Yosry
>>
>> Jasmine tests are failing, can you please fix those and resend the patch.
>>
>> On Fri, Aug 16, 2019 at 11:23 PM Yosry Muhammad <[email protected]>
>> wrote:
>>
>>> Hi hackers,
>>>
>>> Please find attached a patch with minimal refactoring of:
>>> web/pgadmin/tools/sqleditor/static/js/sqleditor.js
>>>
>>> This includes merging 2 redundant functions into one and renaming some
>>> functions to have more expressive and consistent names.
>>>
>>> Please review !
>>> Thanks.
>>> --
>>> *Yosry Muhammad Yosry*
>>>
>>> Computer Engineering student,
>>> The Faculty of Engineering,
>>> Cairo University (2021).
>>> Class representative of CMP 2021.
>>> https://www.linkedin.com/in/yosrym93/
>>>
>>
>>
>> --
>> *Thanks & Regards*
>> *Akshay Joshi*
>>
>> *Sr. Software Architect*
>> *EnterpriseDB Software India Private Limited*
>> *Mobile: +91 976-788-8246*
>>
>

-- 
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/
sqleditorjs_refactor_v2.patch (text/x-patch, 11.8 KB)
diff --git a/web/pgadmin/static/js/sqleditor/query_tool_actions.js b/web/pgadmin/static/js/sqleditor/query_tool_actions.js
index 18e15ecbe..34b6827ad 100644
--- a/web/pgadmin/static/js/sqleditor/query_tool_actions.js
+++ b/web/pgadmin/static/js/sqleditor/query_tool_actions.js
@@ -39,13 +39,8 @@ let queryToolActions = {
   },
 
   executeQuery: function (sqlEditorController) {
-    if(sqlEditorController.is_query_tool) {
-      this._clearMessageTab();
-      sqlEditorController.execute();
-    } else {
-      this._clearMessageTab();
-      sqlEditorController.execute_data_query();
-    }
+    this._clearMessageTab();
+    sqlEditorController.check_data_changes_to_execute_query();
   },
 
   explainAnalyze: function (sqlEditorController) {
@@ -60,7 +55,7 @@ let queryToolActions = {
       settings: this._settings(),
     };
     this._clearMessageTab();
-    sqlEditorController.execute(explainObject);
+    sqlEditorController.check_data_changes_to_execute_query(explainObject);
   },
 
   explain: function (sqlEditorController) {
@@ -76,7 +71,7 @@ let queryToolActions = {
       settings: this._settings(),
     };
     this._clearMessageTab();
-    sqlEditorController.execute(explainObject);
+    sqlEditorController.check_data_changes_to_execute_query(explainObject);
   },
 
   download: function (sqlEditorController) {
diff --git a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
index 2a956a3d7..61d33ce78 100644
--- a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
+++ b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
@@ -2224,7 +2224,7 @@ define('tools.querytool', [
             cm.className += ' bg-gray-lighter opacity-5 hide-cursor-workaround';
           }
           self.disable_tool_buttons(true);
-          self.execute_data_query();
+          self.check_data_changes_to_execute_query();
         }
       },
 
@@ -2265,9 +2265,8 @@ define('tools.querytool', [
         self.on('pgadmin-sqleditor:unindent_selected_code', self._unindent_selected_code, self);
       },
 
-      // Checks if there is any dirty data in the grid before
-      // it executes the sql query in View Data mode
-      execute_data_query: function() {
+      // Checks if there is any dirty data in the grid before executing a query
+      check_data_changes_to_execute_query: function(explain_prefix=null, shouldReconnect=false) {
         var self = this;
 
         // Check if the data grid has any changes before running query
@@ -2279,8 +2278,13 @@ define('tools.querytool', [
           alertify.confirm(gettext('Unsaved changes'),
             gettext('The data has been modified, but not saved. Are you sure you wish to discard the changes?'),
             function() {
-              // Do nothing as user do not want to save, just continue
-              self._run_query();
+              // The user does not want to save, just continue
+              if(self.is_query_tool) {
+                self._execute_sql_query(explain_prefix, shouldReconnect);
+              }
+              else {
+                self._execute_view_data_query();
+              }
             },
             function() {
               // Stop, User wants to save
@@ -2291,12 +2295,17 @@ define('tools.querytool', [
             cancel: gettext('No'),
           });
         } else {
-          self._run_query();
+          if(self.is_query_tool) {
+            self._execute_sql_query(explain_prefix, shouldReconnect);
+          }
+          else {
+            self._execute_view_data_query();
+          }
         }
       },
 
-      // This function makes the ajax call to execute the sql query in View Data mode
-      _run_query: function() {
+      // Makes the ajax call to execute the sql query in View Data mode
+      _execute_view_data_query: function() {
         var self = this,
           url = url_for('sqleditor.view_data_start', {
             'trans_id': self.transId,
@@ -2373,12 +2382,36 @@ define('tools.querytool', [
           .fail(function(e) {
             self.trigger('pgadmin-sqleditor:loading-icon:hide');
             let msg = httpErrorHandler.handleQueryToolAjaxError(
-              pgAdmin, self, e, '_run_query', [], true
+              pgAdmin, self, e, '_execute_view_data_query', [], true
             );
             self.update_msg_history(false, msg);
           });
       },
 
+      // Executes sql query in the editor in Query Tool mode
+      _execute_sql_query: function(explain_prefix, shouldReconnect) {
+        var self = this, sql = '';
+
+        self.has_more_rows = false;
+        self.fetching_rows = false;
+
+        if (!_.isUndefined(self.special_sql)) {
+          sql = self.special_sql;
+        } else {
+          /* If code is selected in the code mirror then execute
+           * the selected part else execute the complete code.
+           */
+          var selected_code = self.gridView.query_tool_obj.getSelection();
+          if (selected_code.length > 0)
+            sql = selected_code;
+          else
+            sql = self.gridView.query_tool_obj.getValue();
+        }
+
+        const executeQuery = new ExecuteQuery.ExecuteQuery(this, pgAdmin.Browser.UserManagement);
+        executeQuery.execute(sql, explain_prefix, shouldReconnect);
+      },
+
       // This is a wrapper to call_render function
       // We need this because we have separated columns route & result route
       // We need to combine both result here in wrapper before rendering grid
@@ -3711,60 +3744,6 @@ define('tools.querytool', [
         }
       },
 
-      // Checks if there is any dirty data in the grid before
-      // it executes the sql query in Query Tool mode
-      execute: function(explain_prefix, shouldReconnect=false) {
-        var self = this;
-
-        // Check if the data grid has any changes before running query
-        if (self.can_edit && _.has(self, 'data_store') &&
-          (_.size(self.data_store.added) ||
-            _.size(self.data_store.updated) ||
-            _.size(self.data_store.deleted))
-        ) {
-          alertify.confirm(gettext('Unsaved changes'),
-            gettext('The data has been modified, but not saved. Are you sure you wish to discard the changes?'),
-            function() {
-              // Do nothing as user do not want to save, just continue
-              self._execute_sql_query(explain_prefix, shouldReconnect);
-            },
-            function() {
-              // Stop, User wants to save
-              return true;
-            }
-          ).set('labels', {
-            ok: gettext('Yes'),
-            cancel: gettext('No'),
-          });
-        } else {
-          self._execute_sql_query(explain_prefix, shouldReconnect);
-        }
-      },
-
-      // Executes sql query in the editor in Query Tool mode
-      _execute_sql_query: function(explain_prefix, shouldReconnect) {
-        var self = this, sql = '';
-
-        self.has_more_rows = false;
-        self.fetching_rows = false;
-
-        if (!_.isUndefined(self.special_sql)) {
-          sql = self.special_sql;
-        } else {
-          /* If code is selected in the code mirror then execute
-           * the selected part else execute the complete code.
-           */
-          var selected_code = self.gridView.query_tool_obj.getSelection();
-          if (selected_code.length > 0)
-            sql = selected_code;
-          else
-            sql = self.gridView.query_tool_obj.getValue();
-        }
-
-        const executeQuery = new ExecuteQuery.ExecuteQuery(this, pgAdmin.Browser.UserManagement);
-        executeQuery.execute(sql, explain_prefix, shouldReconnect);
-      },
-
       /* This function is used to highlight the error line and
        * underlining for the error word.
        */
diff --git a/web/regression/javascript/sqleditor/query_tool_actions_spec.js b/web/regression/javascript/sqleditor/query_tool_actions_spec.js
index e5e486572..487bbb4eb 100644
--- a/web/regression/javascript/sqleditor/query_tool_actions_spec.js
+++ b/web/regression/javascript/sqleditor/query_tool_actions_spec.js
@@ -30,7 +30,7 @@ describe('queryToolActions', () => {
       it('calls the execute function on the sqlEditorController', () => {
         queryToolActions.executeQuery(sqlEditorController);
 
-        expect(sqlEditorController.execute).toHaveBeenCalled();
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalled();
       });
     });
     describe('when the command is being run from the view data view', () => {
@@ -39,10 +39,10 @@ describe('queryToolActions', () => {
         sqlEditorController.is_query_tool = false;
       });
 
-      it('it calls the execute_data_query function on the sqlEditorController', () => {
+      it('it calls the check_data_changes_to_execute_query function on the sqlEditorController', () => {
         queryToolActions.executeQuery(sqlEditorController);
 
-        expect(sqlEditorController.execute_data_query).toHaveBeenCalled();
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalled();
       });
     });
   });
@@ -74,7 +74,7 @@ describe('queryToolActions', () => {
           settings: false,
         };
 
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
 
@@ -100,7 +100,7 @@ describe('queryToolActions', () => {
           summary: true,
           settings: true,
         };
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
 
@@ -128,7 +128,7 @@ describe('queryToolActions', () => {
           settings: false,
         };
 
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
 
@@ -156,7 +156,7 @@ describe('queryToolActions', () => {
           settings: false,
         };
 
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
 
@@ -184,7 +184,7 @@ describe('queryToolActions', () => {
           settings: true,
         };
 
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
   });
@@ -213,7 +213,7 @@ describe('queryToolActions', () => {
           summary: false,
           settings: false,
         };
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
 
@@ -239,7 +239,7 @@ describe('queryToolActions', () => {
           settings: false,
         };
 
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
 
@@ -264,7 +264,7 @@ describe('queryToolActions', () => {
           summary: false,
           settings: false,
         };
-        expect(sqlEditorController.execute).toHaveBeenCalledWith(explainObject);
+        expect(sqlEditorController.check_data_changes_to_execute_query).toHaveBeenCalledWith(explainObject);
       });
     });
   });
@@ -621,8 +621,7 @@ describe('queryToolActions', () => {
       trigger: jasmine.createSpy('trigger'),
       table_name: 'iAmATable',
       is_query_tool: true,
-      execute: jasmine.createSpy('execute'),
-      execute_data_query: jasmine.createSpy('execute_data_query'),
+      check_data_changes_to_execute_query: jasmine.createSpy('check_data_changes_to_execute_query'),
     };
   }
 });
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.