Re: Query Tool JS Minimal Refactoring
Aditya Toshniwal <[email protected]>
| Newsgroups | gmane.comp.db.postgresql.pgadmin.devel |
|---|---|
| Message-ID | <CAM9w-_mggFc4GB+q81+wSmoggxnC50Yu73xpB23DJ+jBdhNsQw@mail.gmail.com> |
Hi Yosry, The v3 patch has changes like: 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 Looks like some other patch got mixed up. Anyway, I've removed these changes and have attached the updated patch (v4) ready for commit. Everything looks good to me. On Fri, Aug 23, 2019 at 2:32 PM Yosry Muhammad <[email protected]> wrote: > 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/ > -- Thanks and Regards, Aditya Toshniwal Software Engineer | EnterpriseDB India | Pune "Don't Complain about Heat, Plant a TREE"
sqleditorjs_refactor_v4.patch
(application/octet-stream, 13.3 KB)
diff --git a/web/pgadmin/static/js/sqleditor/execute_query.js b/web/pgadmin/static/js/sqleditor/execute_query.js
index 26da6914..b8baa410 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 18e15ecb..34b6827a 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 2a956a3d..61d33ce7 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 e5e48657..487bbb4e 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'),
};
}
});