Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server_privileges and server_databases modularized #14465

Merged
merged 12 commits into from Jul 26, 2018

Conversation

Projects
None yet
2 participants
@Piyush3079
Copy link
Contributor

Piyush3079 commented Jul 8, 2018

This pr contains the modular code for pages server_databases and server_privileges.
This pr is slight big because some of the core function required for these two files were copied and then imported.
This pr is in continuation of Mod_Js_Config. Before reviewing this pr, review Mod_Js_Config and merge and then compare this pr with merged one.

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests

@devenbansod devenbansod changed the base branch from master to gsoc-js-refactoring Jul 18, 2018

@devenbansod devenbansod self-assigned this Jul 18, 2018

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch from e324ecd to 898985c Jul 18, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 18, 2018

Codecov Report

Merging #14465 into gsoc-js-refactoring will increase coverage by 0.09%.
The diff coverage is 90.62%.

@@                    Coverage Diff                    @@
##             gsoc-js-refactoring   #14465      +/-   ##
=========================================================
+ Coverage                  50.43%   50.53%   +0.09%     
- Complexity                 14430    14434       +4     
=========================================================
  Files                        500      501       +1     
  Lines                      67858    68006     +148     
=========================================================
+ Hits                       34227    34369     +142     
- Misses                     33631    33637       +6

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch 7 times, most recently from f851344 to d140b2c Jul 18, 2018

@devenbansod
Copy link
Member

devenbansod left a comment

Leaving a few initial review comments and suggestions.

If you could record it, I'd love to see a working screen video though (to confirm nothing is broken as of now), although I would definitely verify the usage at a later stage before merging.

@@ -25,4 +25,4 @@ build:
command: './vendor/bin/phpcs --standard=PMAStandard ./ --report=checkstyle --report-file=cs-data --ignore=*/vendor/*,*/build/*'
analysis:
file: 'cs-data' # The reporter filename
format: 'php-cs-checkstyle' # The supported format by Scrutinizer
format: 'php-cs-checkstyle' # The supported format by Scrutinizer

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

/minor Unrelated change, try to avoid.

// echo "var AJAX_Params = new Array();\n";
// foreach ($header->getDisplay() as $name => $value) {
// Sanitize::printJsValue("AJAX_Params['" . $name . "']", $value);
// }

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

If this is not required, please remove this.

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 22, 2018

Author Contributor

Removed

/* for new js code */
/* extending jquery-ui-timepicker-addon if ($.datepicker)
$.extend($.datepicker._defaults, $.datepicker.regional['']); */

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

If this is not required, please remove it

?>
$.extend($.timepicker._defaults, $.timepicker.regional['']);
} /* if ($.timepicker) */

<?php
/* for new js code */
/* Form validation */
/* extending jquery form validation $.extend($.validator.messages, obj) */

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

If this is not required, please remove it

* @return void
*/
requestHandler: function (event) {
console.log('i am called');

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

/minor stray debug statement

/**
* Copy of functions copied from different files to make them globally
* available so that build does not break during modularisation
*/

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

If these are to be removed later, please add a note in a TODO stating the same.

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 22, 2018

Author Contributor

Added a TODO in the comment itself.

@@ -0,0 +1,719 @@
import { PMA_ajaxShowMessage } from './utils/show_ajax_messages';

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

Try to remove these PMA_ prefixes

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 22, 2018

Author Contributor

Answered above

*/

export const mysql_doc_keyword = {
/* Multi word */

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

Why is this file needed if everything is commented out?

Also, please avoid underscores in exports. Might want to use PascalCase.

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 22, 2018

Author Contributor

This pr does not contain any of the case changes for variables and removal of PMA_ prefix as this pr mainly contains the functions going to use in many files.
I was just thinking to create one or more pr once we create and merge some workable code, which will contain changes like using camelCase for variables, PascalCase for exports and dropping PMA_ prefix.
What do you think about this?

@@ -755,6 +755,26 @@ function () {
Sanitize::printJsValue("PMA_messages['" . $name . "']", $js_message);
}

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

Why are two files required? js/messages.php and js/src/messages.php

Is it possible to do away with one of them?

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 22, 2018

Author Contributor

I have removed the duplicate files and merged the two into one and changed some of the js files accordingly.

@@ -40,10 +40,10 @@ var plugins = [
export default [{
mode: MODE,
entry: {
db_search_new: './js/src/db_search.js'
index_new: './js/src/index.js'
},

This comment has been minimized.

@devenbansod

devenbansod Jul 22, 2018

Member

Why index_new?

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 22, 2018

Author Contributor

The _new.js suffix helps us to identify that for this file we need to use src='webhost+webport/js/dist' and for other .js files we need to use src=js/filename.
When all the files get revamped we can drop the _new suffix and modify Script.php for single entrypoint index.js.

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch 4 times, most recently from e1e2b38 to bdb0ac2 Jul 22, 2018

@@ -1,3 +1,4 @@
{
"presets": ["env"]
"presets": ["env"],
"plugins": ["syntax-dynamic-import"]

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

Babel does not support dynamic imports so to compile that, this plugin is added

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch 2 times, most recently from 31e9061 to 22ec80a Jul 24, 2018

@Piyush3079
Copy link
Contributor Author

Piyush3079 left a comment

General comments for files regarding the changes made.

@@ -0,0 +1,39 @@
<?php

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

File for adding common parameter defined in Header.php to make them available in the global scope as common_param variable and then use this variable to serialize common params data inside the module.

@@ -9,57 +9,6 @@
*
*/

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

Functions removed from server_privileges and copied in functions.js so that they are available in the global scope.

@@ -0,0 +1,763 @@
import { PMA_ajaxShowMessage } from './utils/show_ajax_messages';

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

New ajax handler with old functionality and some new functionality including dynamic imports.
The file is copied from old ajax.js and then modified accordingly.

@@ -0,0 +1,14 @@
/**

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

This file contains the object having the information which js files are needed for a particular php file.

@@ -0,0 +1,78 @@
import { escapeHtml } from '../utils/Sanitise';

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

File containing the PAM_getImage functions. This file is copied from functions.js and then modified accordingly for modular code.

@@ -0,0 +1,100 @@
import { PMA_sprintf } from '../utils/sprintf';

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

PMA_commonParams module for handling common params defined in Header.php

@@ -0,0 +1,10 @@
import { Variables } from './global_variables';

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

Exporting variables from module serialised using global variables.

@@ -0,0 +1,20 @@
import { Variables } from './global_variables';

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

Serialising global variables in the module.

"jquery-mousewheel": "3.1.13",
"jquery-ui": "1.12.1",
"jquery-ui": "^1.12.1",
"jquery-ui-bundle": "^1.12.1-migrate",

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

Jquery-ui does have a bundle so this additional library is added to use jquery-ui with jquery.

@@ -14,20 +14,23 @@
"dependencies": {
"codemirror": "5.37.0",
"jquery": "3.3.1",
"jquery-migrate": "3.0.1",
"jquery-migrate": "3.0.0",

This comment has been minimized.

@Piyush3079

Piyush3079 Jul 24, 2018

Author Contributor

The newer version is not compatible. It is throwing some warnings. So, for now, we can use 3.0.0.

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch from 22ec80a to e66ae54 Jul 25, 2018

@devenbansod

This comment has been minimized.

Copy link
Member

devenbansod commented Jul 25, 2018

@Piyush3079 can you please take a look at the lint errors at https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/407603596?

@devenbansod
Copy link
Member

devenbansod left a comment

Tested locally. Looks good to me.

Please fix the linter errors. I've restarted the selenium test job, let's see if it turns green.

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch 5 times, most recently from 7b4c6ad to e90bd64 Jul 25, 2018

@Piyush3079 Piyush3079 force-pushed the Piyush3079:Mod_Js_Server_Privileges branch 2 times, most recently from 2a56c3b to ae20a38 Jul 25, 2018

Piyush3079 added some commits May 26, 2018

Some functions moved from server_privilages.js to functions.js and ty…
…po error removed

Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
New directory Stucture having modules imported and exported.
This commit needs proper discussion as many of the upcoming changes are based on this commit.
These files contains the working code only.
Right now the code has the functionality to work with both old js files and new js files.
Commeting style is yet to decide
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
server_privilages and server_databases modularized
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
AJAX handler modified and initial loading of the first page handled
Signed-off-by: Piyush Vijay <piyushvijay.1997@gmail.com>
Commented code, debug statements and unused variable in Script.php is…
… removed.

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Review changes for adding file level comments for some files.
Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Replace default validation strings with custom strings to enable loca…
…lization for validation messages with jquery.

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Add todo for code that will not be used in future after revamping all
the files.
Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Modify ajax.js and Header.php for compatibility of both old code and …
…new code.

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Bux fix for onload and teardown of pages on refresh and ajax requests.
Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Bug fix for token mismatch error on server_databases.php page.
Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
Add babel-eslint package to use eslint for features not yet included …
…in eslint.

Signed-Off-By: Piyush Vijay <piyushvijay.1997@gmail.com>
@devenbansod

This comment has been minimized.

Copy link
Member

devenbansod commented Jul 26, 2018

@Piyush3079 I am merging this for now given the shortage of time and dependency of next PR at #14466 on this PR.

But we should take steps to move these changes fast, while also considering the steps needed to build out a workable release from this (i.e. run out of the box from the release zip export).

The selenium tests can't pass right now, since the Login page is itself broken out of the box (since it requires additional setup right now to test out this code)

@devenbansod devenbansod merged commit dfcca3c into phpmyadmin:gsoc-js-refactoring Jul 26, 2018

1 of 3 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
DCO All commits have a DCO sign-off from the author
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.