Add Grunt to compress SMap js files. #1533

linzjax · 2017-05-24T16:19:56Z

[NOTE: Not going into master]

Proposed changes in this pull request

Addresses SMAP: Implement frontend toolchain #1244
setup vagrant provisioning to include grunt.
Added gruntserver file to run with ./runserver.
Setup Gruntfile.js to uglify SMap files as a test
Sorted third party library files into vendor folders
Renamed router/routes/router_mixins to SimpleRouter, CreateRoutes, and RouterMixins respectively.
put datatable and parsley files into their own folders
added jshint to grunt configuration
removed all es6 logic from js files and linting.

When should this PR be merged

Before SMap is put up on staging.

Risks

I'm adding a step to VM set up, as well as a step to deployment. grunt production should be run before deploying to demo/prod. grunt concat should be run when deploying to staging.

Follow-up actions

Double check with adri that this actually works.

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly? It should have the migration label if a new migration is added.
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

oliverroick

This is really good work. Thanks for cleaning up the JS mess and for adding comments to the files...

I still had a few things to moan about though. See below...

oliverroick · 2017-05-29T12:48:36Z

Gruntfile.js

+
+    grunt.registerTask('default', ['concat']);
+    grunt.registerTask('runserver', ['concat', 'watch']);
+    grunt.registerTask('production', ['concat', 'uglify', 'jshint']);


I think running jshint on production is too late. Is there a way to run it on the development server? We can also make it a mandatory test on Travis, but that's a separate task.

That makes sense. I'll add it to the default grunt command (there are two errors currently, and it short circuits "watch").

oliverroick · 2017-05-29T12:57:08Z

Gruntfile.js

+    grunt.loadNpmTasks('grunt-contrib-watch');
+
+    grunt.registerTask('default', ['concat']);
+    grunt.registerTask('runserver', ['concat', 'watch']);


Maybe we should run uglify on dev as well and configure a source map. Currently, when an error happens on dev, I can see the code but it's difficult to tell where it happened because I can only look at the concatenated code.

That's amazing... I was wondering how that worked.

oliverroick · 2017-05-29T12:59:24Z

cadasta/core/management/commands/gruntserver.py

+import atexit
+import signal
+
+from django.contrib.staticfiles.management.commands.runserver import Command\


Ugh, this is ugly. I'd write it like:

from django.contrib.staticfiles.management.commands import runserver class Command(runserver.Command):

oliverroick · 2017-05-29T13:17:38Z

cadasta/core/static/js/rel_tenure.js

-  const relId = target.attr('data-id');
-  target.closest('tbody').find('tr.info').removeClass('info');
-  target.addClass('info');
-  $('input[name="id"]').val(relId);


This works but I don't understand how. We must set the party ID somewhere. Where does this happen? ... just curious.

So, weirdly enough, I looked around and I'm not sure. I believe this was implemented before we added in select2, and now select2 handles it. I couldn't find anywhere in the code that had a table with a select-list id.

Guess we'll find out when it's on staging then ;)

oliverroick · 2017-05-29T13:21:50Z

provision/roles/node/tasks/main.yml

+  become: yes
+  become_user: root
+  file: path=/vagrant/node_modules src=/vagrant/cadasta/core/node_modules state=link follow=yes
+  # when: node_check.stat.exists == False


Can you check back with @amplifi what this does and whether we need it here? If we don't need it then just remove it

Removed the comment.

- setup vagrant provisioning to include grunt. - Added gruntserver file to run with ./runserver. - Setup Gruntfile.js to uglify SMap files as a test - Sorted third party library files into vendor folders - put datatable and parsley files into their own folders - removed all es6 logic from js files - added jshint to grunt configuration - added source mapping from uglified files to originals

oliverroick

Looking good!

- setup vagrant provisioning to include grunt. - Added gruntserver file to run with ./runserver. - Setup Gruntfile.js to uglify SMap files as a test - Sorted third party library files into vendor folders - put datatable and parsley files into their own folders - removed all es6 logic from js files - added jshint to grunt configuration - added source mapping from uglified files to originals

oliverroick requested changes May 29, 2017

View reviewed changes

linzjax force-pushed the feature/map-spa-router-grunt-2 branch from 04572e4 to 3d9f22a Compare May 30, 2017 19:17

linzjax force-pushed the feature/map-spa-router-grunt-2 branch from 3d9f22a to 13253f9 Compare May 31, 2017 16:09

linzjax requested a review from amplifi May 31, 2017 16:10

oliverroick approved these changes Jun 2, 2017

View reviewed changes

linzjax merged commit 65c0d64 into feature/map-spa-router Jun 3, 2017

linzjax deleted the feature/map-spa-router-grunt-2 branch June 3, 2017 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Grunt to compress SMap js files. #1533

Add Grunt to compress SMap js files. #1533

linzjax commented May 24, 2017

oliverroick left a comment

oliverroick May 29, 2017

linzjax May 30, 2017

oliverroick May 29, 2017

linzjax May 30, 2017

oliverroick May 29, 2017

linzjax May 30, 2017

oliverroick May 29, 2017

linzjax May 30, 2017

oliverroick Jun 2, 2017

oliverroick May 29, 2017

linzjax May 31, 2017

oliverroick left a comment

Add Grunt to compress SMap js files. #1533

Add Grunt to compress SMap js files. #1533

Conversation

linzjax commented May 24, 2017

Proposed changes in this pull request

When should this PR be merged

Risks

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

oliverroick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment