Add location adding/editing/deleting capabilities to SMap #1475

linzjax · 2017-05-05T20:50:57Z

[NOTE: This is not going into master. It's going into feature/map-spa-router]
[NOTE #2: This will need to be rebased before pulling it in so I can put together a commit message that doesn't contain "omg I think it's working"]

Proposed changes in this pull request

Adding/editing/deleting locations are intercepted and handled through ajax
Replaced leaflet-draw with leaflet-edit
Completely redesigned location editing to work with SMap
Fixed formatting in js files
Upgraded django-leaflet
Addresses SMAP: Location adding/editing/deleting #1421, Location geometries are not stored #1378 (sort of. Added a modal to let the user know), SMAP: Implement replacement for LeafletWidget #1124, and Editing a point-type location shows zoom level 0 #718

When should this PR be merged

After someone breaks all of the things I thought I'd fixed
This is a water testing PR, I'm pretty sure there are going to be issues, I just need another set of eyes
Once it's merged into feature/map-spa-router, we can test that, clean it up, and then merge it into feature/map-spa, then... maybe... think about merging it into master? 🙀

Risks

We should discuss whether or not we want clicking on the big "save" button to cascade to clicking on the map "save" button when editing geometries.
Brian was right when he said there were a lot of "gotcha"s. When testing this, make sure you behave like a really obnoxious user and make your behavior as erratic as possible. For example: I found a bug when I started to add a location, clicked save on the map, changed my mind and canceled the whole form. I think added another location and saved it. Then decided I didn't want that either and tried to delete it. Glitch occurred.

Follow-up actions

Compare to master, making sure we integrated all changes that have occurred since we started out on this journey.
Remove any leftover comments, unnecessary/redundant code.
Me buying a beer for Brian in Madrid because I'm super glad I didn't have to do this bit 😸

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

Just two little things

oliverroick · 2017-05-09T13:07:48Z

cadasta/core/static/js/smap/router_mixins.js

+        }
+    },
+
+    // setCurrentLocationStyle: function () {


This can be removed I guess?

oliverroick · 2017-05-09T13:09:10Z

requirements/common.txt

@@ -10,7 +10,7 @@ django-parsley==0.6
 django-formtools==1.0
 django-countries==3.4.1
 django-sass-processor==0.3.4
-django-leaflet-cadasta==0.21.0
+django-leaflet==0.22.0


We haven't fully removed django-leaflet, right? It's still used to edit project extents?

Yeah that's right. We also use the leaflet_tags to load leaflet.js and related css for smap rather than pulling them in from a cdn since they're there locally anyway.

oliverroick

🎖

bjohare · 2017-05-11T09:28:38Z

Weird, weird, weird.. @linzjax have you ever seen these outsized vertex handles.. just noticed them yesterday? They appear in Chromium & Google Chrome 58 (ubuntu 14.04). Seem fine in FF 53.0 (ubuntu 14.04)

linzjax · 2017-05-11T15:26:47Z

@bjohare Yeah I noticed those, but I didn't realize it was a bug. Katrina was excited about them, so does that make it a "feature"?

bjohare · 2017-05-11T15:34:07Z

feature/bug/bug/feature.. kinda ugly though and the more vertices a polygon has the uglier its going to get..

linzjax · 2017-05-11T15:47:25Z

Alright, adjusted the map height in mobile and tested it on my phone. It does, in fact, work on mobile. 🥇

bjohare · 2017-05-11T15:49:30Z

Yahoooo!

linzjax · 2017-05-12T13:34:00Z

Alright, I'm going to rebase/squash and then merge in into feature/map-spa-router unless anyone has any objection? Speak now or forever hold your peace

wonderchook · 2017-05-12T15:00:08Z

@linzjax I think it is time to merge away!

- updated django-leaflet version. - removed l.draw and switched to l.editable - geometry styling moved to separate file - added location editing forms to routes to allow for ajax interception - removed location popups

- updated django-leaflet version. - removed l.draw and switched to l.editable - geometry styling moved to separate file - added location editing forms to routes to allow for ajax interception - removed location popups fixing things after rebase

linzjax requested review from oliverroick and bjohare May 5, 2017 20:51

oliverroick requested changes May 9, 2017

View reviewed changes

oliverroick approved these changes May 10, 2017

View reviewed changes

bjohare approved these changes May 11, 2017

View reviewed changes

linzjax force-pushed the feature/map-spa-router-locations-linz branch from a4443fd to f52db5b Compare May 12, 2017 13:32

linzjax force-pushed the feature/map-spa-router branch from b3dbf5a to 5955e40 Compare May 12, 2017 13:32

Implemented location adding/editing/deleting for SMap.

4f54398

- updated django-leaflet version. - removed l.draw and switched to l.editable - geometry styling moved to separate file - added location editing forms to routes to allow for ajax interception - removed location popups

linzjax force-pushed the feature/map-spa-router-locations-linz branch from f52db5b to 4f54398 Compare May 12, 2017 19:47

linzjax merged commit 23c5601 into feature/map-spa-router May 12, 2017

linzjax deleted the feature/map-spa-router-locations-linz branch May 12, 2017 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add location adding/editing/deleting capabilities to SMap #1475

Add location adding/editing/deleting capabilities to SMap #1475

linzjax commented May 5, 2017

oliverroick left a comment

oliverroick May 9, 2017

linzjax May 9, 2017

oliverroick May 9, 2017

bjohare May 9, 2017

oliverroick left a comment

bjohare commented May 11, 2017

linzjax commented May 11, 2017

bjohare commented May 11, 2017

linzjax commented May 11, 2017

bjohare commented May 11, 2017

linzjax commented May 12, 2017

wonderchook commented May 12, 2017

Add location adding/editing/deleting capabilities to SMap #1475

Add location adding/editing/deleting capabilities to SMap #1475

Conversation

linzjax commented May 5, 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

oliverroick May 9, 2017

Choose a reason for hiding this comment

linzjax May 9, 2017

Choose a reason for hiding this comment

oliverroick May 9, 2017

Choose a reason for hiding this comment

bjohare May 9, 2017

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment

bjohare commented May 11, 2017

linzjax commented May 11, 2017

bjohare commented May 11, 2017

linzjax commented May 11, 2017

bjohare commented May 11, 2017

linzjax commented May 12, 2017

wonderchook commented May 12, 2017