Implement form support in Vue.js/REST API

[NateWr]

The commit history on this is very large, so I will summarize the PRs and discussion here.

Summary

These PRs replace the context and site settings forms. A set of Vue.js components controls forms on the client side. Forms are submitted to the API, which uses service classes to validate, sanitize, get, create and update objects.

Significant infrastructural changes have been put in place, which I hope to consolidate into best practices used throughout the application. Includes:

• Define and document entities with json-schema.
• Replace our validation with Laravel's validation library.
• Service class methods to sanitize and validate entities based on their json-schema.
• A set of form and field components to configure and load forms on the frontend.

I've written up a rough introduction to the new schema-based architecture.

PRs:

https://github.com/pkp/pkp-lib/pull/3931 https://github.com/pkp/ojs/pull/2067 https://github.com/pkp/omp/pull/614 https://github.com/pkp/ui-library/pull/20 https://github.com/pkp/ojs-user-guide/pull/20 https://github.com/pkp/omp-user-guide/pull/2 https://github.com/pkp/customBlockManager/pull/46 https://github.com/pkp/citationStyleLanguage/pull/44 https://github.com/pkp/orcidProfile/pull/49 https://github.com/pkp/staticPages/pull/42 https://github.com/pkp/tinymce/pull/41 https://github.com/pkp/usageStats/pull/6

Theme PRs:

https://github.com/NateWr/bootstrap3/pull/101 https://github.com/NateWr/criticalTimes/pull/4

Discussion

• Alec's outline of desirable outcomes
• Initial questions/discussion. Responses: Namespaces, Naming patterns
• Build size
• Ready for second code review

[asmecher]

So a couple of notes about the current Form Builder Vocabulary structure. Overall it's been inflexible and weird and unpleasant to work with, but there are a few things it has done decently:

• Abstracted away labeling and kept the required markup minimal (and accessibility standardized, if not wonderful)
• Managed details like TinyMCE support with the flip of an attribute
• Managed escaping/XSS (not hard to do with e.g. raw HTML, but needs to be done)
• Especially multilingual support. This probably justifies its own meeting.

For better or for worse it did shrink a lot of multifarious markup down to a much trimmer and more standardized set.

[NateWr]

I have a few questions and things for discussion:

1. I'd like to get the new Form and ValidatorFactory classes to take advantage of autoload, but I am not very familiar with that all works. Can you suggest some code to implement that so we can remove the import statements?

2. What do you think about having common method and hook names for the service class methods? For example, at the moment we use the following:

• SubmissionService::getSubmission(), ::getSubmissions()
• ContextService::getContext(), ::getContexts()
• SiteService::getSite()

What would you think about renaming them to this:

• SubmissionService::getOne(), ::getMany()
• ContextService::getOne(), ::getMany()
• SiteService::getOne()

I would think we'd also use common names for addContext (add), editContext (edit) and deleteContext (delete). This would make the service class API more consistent across different objects, so that new developers can apply their knowledge from one to another.

Along these same lines, do you have any experience using interfaces or traits? I wondered if this would be a good opportunity to introduce things like this to enforce consistency across our service classes.

3. On a similar note, how would you feel about shortening Services::instance()->get('xxx') to Services::get('xxx')?

4. The build size on the JS package is getting large. I took a look at where we're wasting, and it's in some of our third-party components. Tooltip, FieldColor, FieldOptions and FieldUpload together account for 181kb. This is just a note to say I'm aware it's getting big, and we'll have to think about ways to keep this down over time, either through code splitting or ditching some of the most expensive dependencies. I also have some refactoring ideas for the list panels which might reduce size a bit.

5. The forms highlighted shortcomings in the UI Library for documenting components. The documentation on them is not sufficient yet. I have an idea for restructuring things a bit that shouldn't be too much work and would still help us document things in detail. I'll work on that.

6. You may panic a bit when you see the total lines added/removed from the ojs/pkp-lib/ui-library pulls. 9545 of those lines are just adding the package-lock.json. When you remove those, we come in for a total of -1438 lines. :phew:

[NateWr]

OK, I've updated the initial comment with a summary and list of PRs, so that they don't get lost as the commit mentions build up. I've also posted a bunch of initial questions/comments.

The tests will take ages to get through. But @asmecher can you begin code review whenever you're ready? I will work on OMP once we've narrowed things down with the initial code review.

[NateWr]

The build size with all of the forms is getting big. I did some tests to try to identify where the size is coming from. Some numbers:

• 598kb: Build size with this PR
• 210kb: All the Field components
• 181kb: Sum of Tooltip, FieldColor, FieldOptions (w/ draggable), FieldUpload (w/ dropzone.js)
• 292kb: Build size of master (no form elements)
• 87kb: ListPanels and other non-form components
• 188kb: Build size of just Vue.js with no components
• 98.5kb: Build size of just Vue.js when we remove babel-polyfill from webpack config (this is close to the size of the CDN copy they provide of vue.min.js)
• 202kb: Build size of master when we remove babel-polyfill from webpack config

Based on this I draw a couple of conclusions:

1. babel-polyfill is causing a third to half of the overall file size. Some polyfill will be necessary to support older browsers, but I wonder if we can improve this at all? Our current .babelrc file is:

{
  "presets": [
    ["env", { "modules": false }],
    "stage-2"
  ],
  "plugins": ["transform-runtime"],
  "comments": false,
}

I confess to being confused by how to configure babel. I think we need to support IE10 or at the very least IE11. Because our software is used heavily in university IT networks, which are slow to update, and developing countries, which may not be able to afford updates, we probably need to be pretty generous in supporting back browsers. But I'm not sure if IE10 is still on that list. Can we change the babel config to improve our build size? Should we be using the browserslist approach for config?

2. The off-the-shelf components we're using are a big source of build size inflation. The color picker library alone is ~70kb. Dropzone.js is ~50kb. We'll eventually be able to split the built packages based on what's needed for each part of the app. The color picker, for example, doesn't need to be loaded outside of the settings page. It's a good reminder, though, to be cautious in what off-the-shelf components we use.

cc @alex-wreschnig

[asmecher]

On namespaces: here's a partial snippet that'll move one class into a namespace (and one invocation -- all invocations will need to be similarly adapted):

diff --git a/classes/form/Form.inc.php b/classes/form/Form.inc.php
index f35d792..7f0e3bf 100644
--- a/classes/form/Form.inc.php
+++ b/classes/form/Form.inc.php
@@ -45,7 +45,6 @@ import('lib.pkp.classes.form.validation.FormValidatorUri');
 import('lib.pkp.classes.form.validation.FormValidatorUrl');
 import('lib.pkp.classes.form.validation.FormValidatorLocaleUrl');
 import('lib.pkp.classes.form.validation.FormValidatorISSN');
-import('lib.pkp.classes.form.validation.FormValidatorORCID');

 class Form {

diff --git a/classes/form/validation/FormValidatorORCID.inc.php b/classes/form/validation/FormValidatorORCID.inc.php
index 4f0ed15..52a06e1 100644
--- a/classes/form/validation/FormValidatorORCID.inc.php
+++ b/classes/form/validation/FormValidatorORCID.inc.php
@@ -13,9 +13,10 @@
  * @brief Form validation check for ORCID iDs.
  */

-import('lib.pkp.classes.form.validation.FormValidator');
+namespace PKP\form\validation;

-class FormValidatorORCID extends FormValidator {
+import('lib.pkp.classes.form.validation.FormValidator');
+class FormValidatorORCID extends \FormValidator {
    /**
     * Constructor.
     * @param $form Form the associated form
@@ -25,7 +26,7 @@ class FormValidatorORCID extends FormValidator {
     */
    function __construct($form, $field, $type, $message) {
        import('lib.pkp.classes.validation.ValidatorORCID');
-       $validator = new ValidatorORCID();
+       $validator = new \ValidatorORCID();
        parent::__construct($form, $field, $type, $message, $validator);
    }
 }
diff --git a/classes/user/form/PublicProfileForm.inc.php b/classes/user/form/PublicProfileForm.inc.php
index 8687c5b..5c08339 100644
--- a/classes/user/form/PublicProfileForm.inc.php
+++ b/classes/user/form/PublicProfileForm.inc.php
@@ -30,7 +30,7 @@ class PublicProfileForm extends BaseProfileForm {
        parent::__construct('user/publicProfileForm.tpl', $user);

        // Validation checks for this form
-       $this->addCheck(new FormValidatorORCID($this, 'orcid', 'optional', 'user.orcid.orcidInvalid'));
+       $this->addCheck(new PKP\form\validation\FormValidatorORCID($this, 'orcid', 'optional', 'user.orcid.orcidInvalid'));
        $this->addCheck(new FormValidatorUrl($this, 'userUrl', 'optional', 'user.profile.form.urlInvalid'));
    }

[asmecher]

On point 2 (function name standardization), absolutely, we should work towards removing the name of the entity from the service class function name since the service class name makes it explicit. We did this with DAOs a long time ago -- getById replaced e.g. getArticleById -- and should probably continue that habit elsewhere.

On point 3 (shorter invocation syntax), yes, that's a lot less symbol salad, thanks.

[NateWr]

@asmecher I'm running into a couple of JS validation issues that I can't figure out how to address.:

1. ModalHandler.js is complaining that pkp is not defined: https://travis-ci.org/pkp/ojs/jobs/448383107#L1052. But it's a global that I use in other handlers without any validation issues (example). Do you know what's causing the validation error here?

2. I'm getting a validation error that a property is not defined on options: https://travis-ci.org/pkp/ojs/jobs/448383107#L1099. But I can't find examples of any of the other options in ModalHandler being defined. Do you know where I'm supposed to define that?

3. I'm not sure how to define a function inside of an item in an array: https://travis-ci.org/pkp/ojs/jobs/448383107#L1103. I've defined the array in closure-externs.js. I looked for documentation on how to define a function that is a property of an item in an array, but couldn't find anything. Do you have any idea?

[asmecher]

ModalHandler.js is complaining that pkp is not defined: https://travis-ci.org/pkp/ojs/jobs/448383107#L1052. But it's a global that I use in other handlers without any validation issues (example). Do you know what's causing the validation error here?

Off the top of my head: try $.pkp instead of pkp. That should validate.

I'm getting a validation error that a property is not defined on options: https://travis-ci.org/pkp/ojs/jobs/448383107#L1099. But I can't find examples of any of the other options in ModalHandler being defined. Do you know where I'm supposed to define that?

See e.g. the @type {{ ... }} syntax here: https://github.com/pkp/pkp-lib/blob/034b6f787a44c50c12c9d15eb65a0dbc0bc47f0e/js/controllers/modal/ModalHandler.js#L46..L47

I'm not sure how to define a function inside of an item in an array: https://travis-ci.org/pkp/ojs/jobs/448383107#L1103. I've defined the array in closure-externs.js. I looked for documentation on how to define a function that is a property of an item in an array, but couldn't find anything. Do you have any idea?

Sometimes you have to use explicit typecasts to get Crockford to stop glaring at you. Something like...

1. Add a new instance variable to the var definition (which has to be at the top of the function, and only one var, thank you Crockford)

2. Change your statement:

   $.pkp.registry._instances[id].$destroy();

   ...to something like...

   instance = /** @type JQueryObject */ ($.pkp.registry._instances[id]);
   instance.$destroy();

[NateWr]

Thanks! I always forget about the inline /** @type ... technique. I'll run those now.

With the first one, its not actually $.pkp. It's a separate global var, just pkp, where some our Vue library bits are stored (the event bus, a component registry, etc). You can see it's definition in closure-externs.js here:

https://github.com/NateWr/pkp-lib/blob/i3594_forms/tools/closure-externs.js#L308-L320

Is there something else I need to do to use a global var and pass the linting? Do you have any objection to me using the ignore syntax here?

/* jshint ignore:start */
if (typeof pkp.registry._instances[id] !== 'undefined') {
/* jshint ignore:end */

Or would you prefer I put in something like this in the function's var statement?

var pkp = pkp || {}

[NateWr]

@asmecher I was finally able to work around the last validation error by using /*global pkp*/ at the top of ModalHandler.js, which I understand is a JSLint feature.

I don't see any more validation errors in the Travis report, but the validation tests are still returning a failture: https://travis-ci.org/pkp/ojs/builds/451306005?utm_source=github_status&utm_medium=notification. Have you ever seen this before?

[asmecher]

@NateWr, check line 1119/1120 of the log:

[NateWr]

@asmecher :+1: that was it!

(For future reference, don't use sudo when running npm because it's not available to the system account.)

[NateWr]

I've addressed all of the comments in the first code review and fixed the tests (except for the DateStringNormalizerFilter one). I think it's ready for a second code review. I've squashed all the changes into two commits:

https://github.com/NateWr/pkp-lib/commit/02e6531ce0521e786b1cb51246baf7d06237c3fb https://github.com/NateWr/ojs/commit/db1462f0be95625853066ae3a6ebf9eb856e9f4e

In particular, you'll want to look at:

• Introduced namespaces for forms and fields.
• Changed the OJS namespace to APP so it can be used in pkp-lib for both applications.
• Introduced interfaces and traits for the service classes.
• Restored that $forceReload argument, which we use to allow plugins to modify the schema of a context.
• There was a tricky bug with disable_path_info and PUT, POST and DELETE requests. See the commit messages for more info.

I'm now going to start working on OMP.

(Edit: Well the tests were working! They're getting hung up on something now so I'll take another look. But it's still ready for a second code review.)

[asmecher]

Looks good, @NateWr. Hmm, I'm not sure the trait's single function justifies its own file -- what about making a factory method e.g. as a static function of DBResultRange instead?

I hope, when you need it, you will find this message and it will save you the day that I just had.

Noted :scream: and thanks for chasing this down. One day I'd like to get rid of path_info_disabled -- it's cost a lot of time and sweat to keep it functioning -- but I do still encounter journals in the wild that use it.

[asmecher]

Also, I like the conventions we're starting to introduce here. If you have some in particular you'd like to see us adopt elsewhere in the codebase, please make note!

[asmecher]

I see you're mapping Exceptions into fatalError calls -- that's consistent with what we've done before, but just FYI, I'm OK with using Exceptions in the future.

[asmecher]

I've had a skim over it again and all systems go! When you're ready for OMP, let me know.

[NateWr]

@asmecher I've added a PR for OMP to the original post in this issue and the tests are passing. :tada: Ready for (hopefully) a final code review!

[asmecher]

Excellent work, @NateWr -- well considered and handled en masse with consistency and attention to detail. I'm sure there will be some breakage and adjustment once this gets merged but IMO the master branch is ready for it. Please go ahead and merge -- ensuring that there are stable-3_1_2 branches in the relevant repos (e.g. plugins) before you do, as this contains breaking changes -- and if you don't mind, I'll give you a few minutes during the next dev call to orient the team briefly on what they'll need to know. :tada:!

[NateWr]

Merged! :tada: