Re: Query Tool JS Minimal Refactoring
Yosry Muhammad <[email protected]>
| Newsgroups | gmane.comp.db.postgresql.pgadmin.devel |
|---|---|
| Message-ID | <CAFSMqn894khK+emax+99r7SMtokF3N=4_xDgWzGeZoXmN=6jzA@mail.gmail.com> |
Please find an updated patch attached. On Fri, Aug 23, 2019 at 7:57 AM Aditya Toshniwal < [email protected]> wrote: > Hi Yosry, > > This breaks the reconnect for query tool. Open a query tool, execute some > query and then restart the python server. Go to the query tool and click > execute. It will show a warning, on continuing it should connect again. It > throws exception in browser console: > Uncaught TypeError: Cannot read property 'apply' of undefined > at Object.eval (VM69935 sqleditor.js:1769) > at Object.callback (alertify.js:3347) > at triggerCallback (alertify.js:1220) > at Object.buttonsClickHandler (alertify.js:1241) > at HTMLDivElement.eval (alertify.js:299) > > On Thu, Aug 22, 2019 at 11:44 PM Yosry Muhammad <[email protected]> > wrote: > >> 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/ >> > > > -- > Thanks and Regards, > Aditya Toshniwal > Software Engineer | EnterpriseDB India | Pune > "Don't Complain about Heat, Plant a TREE" > -- *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_v3.patch
(text/x-patch, 15.7 KB)
diff --git a/docs/en_US/release_notes_4_13.rst b/docs/en_US/release_notes_4_13.rst
index bc21dfeca..a0a3a38ef 100644
--- a/docs/en_US/release_notes_4_13.rst
+++ b/docs/en_US/release_notes_4_13.rst
@@ -22,5 +22,4 @@ Bug fixes
| `Issue #2706 <https://redmine.postgresql.org/issues/2706>`_ - Added ProjectSet icon for explain module.
| `Issue #2828 <https://redmine.postgresql.org/issues/2828>`_ - Added Gather Merge, Named Tuple Store Scan and Table Function Scan icon for explain module.
| `Issue #4643 <https://redmine.postgresql.org/issues/4643>`_ - Fix Truncate option deselect issue for compound triggers.
-| `Issue #4644 <https://redmine.postgresql.org/issues/4644>`_ - Fix length and precision enable/disable issue when changing the data type for Domain node.
-| `Issue #4650 <https://redmine.postgresql.org/issues/4650>`_ - Fix SQL tab issue for Views. It's a regression of compound triggers.
\ No newline at end of file
+| `Issue #4644 <https://redmine.postgresql.org/issues/4644>`_ - Fix length and precision enable/disable issue when changing the data type for Domain node.
\ No newline at end of file
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py
index 4bc357c83..cfefa5314 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py
@@ -879,7 +879,6 @@ class ViewNode(PGChildNodeView, VacuumSettings):
Get all compound trigger nodes associated with view node,
generate their sql and render into sql tab
"""
- SQL_data = ''
if self.manager.server_type == 'ppas' \
and self.manager.version >= 120000:
@@ -889,6 +888,7 @@ class ViewNode(PGChildNodeView, VacuumSettings):
# Define template path
self.ct_trigger_temp_path = 'compound_triggers'
+ SQL_data = ''
SQL = render_template("/".join(
[self.ct_trigger_temp_path,
'sql/{0}/#{1}#/nodes.sql'.format(
@@ -949,7 +949,7 @@ class ViewNode(PGChildNodeView, VacuumSettings):
SQL_data += '\n'
SQL_data += SQL
- return SQL_data
+ return SQL_data
def get_trigger_sql(self, vid):
"""
diff --git a/web/pgadmin/static/js/sqleditor/execute_query.js b/web/pgadmin/static/js/sqleditor/execute_query.js
index 26da69148..b8baa410d 100644
--- a/web/pgadmin/static/js/sqleditor/execute_query.js
+++ b/web/pgadmin/static/js/sqleditor/execute_query.js
@@ -224,22 +224,22 @@ class ExecuteQuery {
}
if (this.userManagement.isPgaLoginRequired(httpMessage.response)) {
- this.sqlServerObject.saveState('execute', [this.explainPlan]);
+ this.sqlServerObject.saveState('check_data_changes_to_execute_query', [this.explainPlan]);
this.userManagement.pgaLogin();
}
if (httpErrorHandler.httpResponseRequiresNewTransaction(httpMessage.response)) {
- this.sqlServerObject.saveState('execute', [this.explainPlan]);
+ this.sqlServerObject.saveState('check_data_changes_to_execute_query', [this.explainPlan]);
this.sqlServerObject.initTransaction();
}
if (this.wasDatabaseConnectionLost(httpMessage)) {
- this.sqlServerObject.saveState('execute', [this.explainPlan]);
+ this.sqlServerObject.saveState('check_data_changes_to_execute_query', [this.explainPlan]);
this.sqlServerObject.handle_connection_lost(false, httpMessage);
}
if(this.isCryptKeyMissing(httpMessage)) {
- this.sqlServerObject.saveState('execute', [this.explainPlan]);
+ this.sqlServerObject.saveState('check_data_changes_to_execute_query', [this.explainPlan]);
this.sqlServerObject.handle_cryptkey_missing();
return;
}
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'),
};
}
});