Add user measurement system #1610

laura-barluzzi · 2017-06-26T21:28:59Z

Proposed changes in this pull request

This PR contains a migration. The migration file on this document starts as '0005_', therefore it should be merged after PR #1569 (which in turns has a migration file starting as '0004_'.

This PR replicates almost entirely the features in PR #1569 and is part of the epic issue User Dashboard #1491.

In settings/default.py I created global variables for available measurement systems and a default one. In this way it is easy to make future additions and/or changes to the available options.

  MEASUREMENT_DEFAULT = 'metric'
  MEASUREMENTS = [
      ('metric', _('Metric')),
      ('imperial', _('Imperial')),
  ]

In accounts/models.py I created a field where to store the preferred choice (or the default) value.

    measurement = models.CharField(max_length=20,
                                   choices=settings.MEASUREMENTS,
                                   default=settings.MEASUREMENT_DEFAULT)

In accounts/tests/test_forms.py I added the measurement field wherever it was required and inside the existing function def test_update_user in order to test when the value is valid.
In accounts/tests/test_forms.py I added def test_update_user_with_invalid_measurement.
API: add measurement ChoiceField in the serializers.UserSerializer class
API: Add measurement case tests in accounts/tests/test_serializers.py

When should this PR be merged

As soon as it has been reviewed.

Risks

no risks, but it may cause many unfounded merging conflicts with the existing code in PR Add account user language #1569 since both PRs have changes located in the same files and lines.
similarly, the migration file from this PR starts with 0004_, the same as the migration file in PR Add account user language #1569.

Follow-up actions

When this and the Automatic calculation of a polygon area Automatic calculation of a polygon area #1534 PRs are both merged we should make a PR from branch feature/area_calculation_with_user_measurement. This will enable to combine the functionalities provided by both PRs.

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 looks good, I just have a few small requests. See below.

oliverroick · 2017-06-28T07:46:40Z

cadasta/accounts/serializers.py

@@ -73,6 +74,8 @@ class UserSerializer(SanitizeFieldSerializer,
                      "email address")
        )]
    )
+    measurement = ChoiceField(choices=next(zip(*settings.MEASUREMENTS)),


I think, this will work without next and zip

oliverroick · 2017-06-28T07:49:14Z

cadasta/accounts/migrations/0004_add_measurement_field.py

@@ -0,0 +1,25 @@
+# -*- coding: utf-8 -*-


We need to make sure that the name doesn't conflict with the migration from #1569, once #1569 is merged and this branch is rebased. I believe it shouldn't be a problem but I want to be sure.

I change this migration file with a starting 0005_ so that it should be merged after #1569 with has a 0004_ instead.

oliverroick · 2017-06-28T07:50:16Z

cadasta/accounts/forms.py

@@ -92,6 +93,15 @@ def clean_username(self):
                _("Username cannot be “add” or “new”."))
        return username

+    def clean_measurement(self):
+        measurements = next(zip(*settings.MEASUREMENTS))


next and zip are not needed.

oliverroick · 2017-06-28T07:52:53Z

cadasta/accounts/forms.py

@@ -92,6 +93,15 @@ def clean_username(self):
                _("Username cannot be “add” or “new”."))
        return username

+    def clean_measurement(self):


You could define a ChoiceField instead (similar to what you did with the serializer for the user language). Using the invalid_choice keyword, you can also define a custom error message.

Great. I tried this and it simplified a lot everything:

no need at all of def clean_measurement(self):

no need to test the ValidationError raise with pytest.

I am going to do the same thing for Add account user language #1569 -- In that PR I added a ChoiceField only in the serializers.py, not in forms.py yet.

bjohare

Other than @oliverroick's comments on the code/migration it looks good to me. Minor point, but a manual test case should probably be added to the manual test case spreadsheet.
https://docs.google.com/spreadsheets/d/1JmKVAmdQgjdzg8YhrSDtfBtWckkv7aq4-ZIZCmwbbwU/edit?usp=sharing

oliverroick

👍

laura-barluzzi · 2017-06-29T18:24:01Z

@bjohare I added 2 entries on the document: lines 10-11. One for changing language and one for changing measurement. I didn't know what to put in the column Link to test case in cadasta-test though. I placed these 2 lines close to similar test cases.

I noticed that there isn't the compulsory step to enter the password before 5. Click "Update Profile". I figure the password field has been added recently.

laura-barluzzi · 2017-06-29T22:11:02Z

@oliverroick and @bjohare could you please review this again? No code has been changed, I resolved all merging conflicts but along the process your reviews got dismissed. Cheers 👍

bjohare · 2017-06-30T09:54:40Z

@laura-barluzzi yeah, the manual test case stuff is a little out of date.. We haven't been very good about updating the spreadsheet when submitting PR's. @seav is working on the cadasta-test stuff so there are some broken links there too. I'd suggest just adding in missing steps as you find them.

bjohare

This looks fine. If we're going to be adding any more user profile stuff, eg profile pictures etc., we should consider breaking that out into a separate UserProfle model with a 1:1 relationship with User.

laura-barluzzi · 2017-06-30T15:14:04Z

@bjohare sure. I noticed that again the review was dismiss when I rebased, squashed and pushed.. should I just avoid to squash commits from now on?

bjohare · 2017-06-30T15:57:50Z

@laura-barluzzi we still haven't settled on a decision about automatic v's manual review dismissal so I think its fine to keep rebasing

laura-barluzzi added API bug PR: bugfix PR: migration and removed bug PR: bugfix labels Jun 26, 2017

oliverroick requested changes Jun 28, 2017

View reviewed changes

oliverroick requested a review from bjohare June 28, 2017 08:33

bjohare reviewed Jun 28, 2017

View reviewed changes

laura-barluzzi force-pushed the feature/user_measurement branch from 6f67af2 to 8761a53 Compare June 28, 2017 22:01

oliverroick previously approved these changes Jun 29, 2017

View reviewed changes

amplifi added PR: needs review PR: needs testing labels Jun 29, 2017

laura-barluzzi dismissed oliverroick’s stale review via d805142 June 29, 2017 20:01

laura-barluzzi force-pushed the feature/user_measurement branch 6 times, most recently from 7d5dfcc to dd41bc1 Compare June 29, 2017 21:50

oliverroick added PR: approved PR: needs review and removed PR: needs review PR: approved labels Jun 30, 2017

bjohare previously approved these changes Jun 30, 2017

View reviewed changes

laura-barluzzi dismissed bjohare’s stale review via 44f5675 June 30, 2017 15:12

laura-barluzzi force-pushed the feature/user_measurement branch from dd41bc1 to 44f5675 Compare June 30, 2017 15:12

bjohare previously approved these changes Jun 30, 2017

View reviewed changes

laura-barluzzi dismissed bjohare’s stale review via 0cf22c9 June 30, 2017 16:36

laura-barluzzi force-pushed the feature/user_measurement branch from 44f5675 to 0cf22c9 Compare June 30, 2017 16:36

amplifi previously approved these changes Jul 4, 2017

View reviewed changes

amplifi added PR: approved and removed PR: needs review PR: needs testing labels Jul 4, 2017

add user preferred measurement system

e2f88dd

amplifi dismissed their stale review via e2f88dd July 4, 2017 14:19

amplifi force-pushed the feature/user_measurement branch from 0cf22c9 to e2f88dd Compare July 4, 2017 14:19

amplifi approved these changes Jul 4, 2017

View reviewed changes

amplifi merged commit f74b787 into master Jul 4, 2017

amplifi deleted the feature/user_measurement branch July 4, 2017 14:35

jnordling mentioned this pull request Jul 6, 2017

Automatic calculation of a polygon area #1534

Merged

20 tasks

laura-barluzzi mentioned this pull request Aug 8, 2017

Fixes #1666 showing calculated areas with user measurement system #1706

Merged

46 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user measurement system #1610

Add user measurement system #1610

laura-barluzzi commented Jun 26, 2017 •

edited

Loading

oliverroick left a comment

oliverroick Jun 28, 2017

oliverroick Jun 28, 2017

laura-barluzzi Jun 28, 2017

oliverroick Jun 28, 2017

oliverroick Jun 28, 2017

laura-barluzzi Jun 28, 2017

bjohare left a comment

oliverroick left a comment

laura-barluzzi commented Jun 29, 2017

laura-barluzzi commented Jun 29, 2017 •

edited

Loading

bjohare commented Jun 30, 2017

bjohare left a comment

laura-barluzzi commented Jun 30, 2017

bjohare commented Jun 30, 2017

Add user measurement system #1610

Add user measurement system #1610

Conversation

laura-barluzzi commented Jun 26, 2017 • edited Loading

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 Jun 28, 2017

Choose a reason for hiding this comment

oliverroick Jun 28, 2017

Choose a reason for hiding this comment

laura-barluzzi Jun 28, 2017

Choose a reason for hiding this comment

oliverroick Jun 28, 2017

Choose a reason for hiding this comment

oliverroick Jun 28, 2017

Choose a reason for hiding this comment

laura-barluzzi Jun 28, 2017

Choose a reason for hiding this comment

bjohare left a comment

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment

laura-barluzzi commented Jun 29, 2017

laura-barluzzi commented Jun 29, 2017 • edited Loading

bjohare commented Jun 30, 2017

bjohare left a comment

Choose a reason for hiding this comment

laura-barluzzi commented Jun 30, 2017

bjohare commented Jun 30, 2017

laura-barluzzi commented Jun 26, 2017 •

edited

Loading

laura-barluzzi commented Jun 29, 2017 •

edited

Loading