Fix #1018: Make project details editing work again #1019

seav · 2016-12-22T13:38:32Z

Proposed changes in this pull request

Fix Editing project details does not succeed #1018:
- Corrected call to super().post().
- Update unit tests. In particular, test_post_with_blocked_questionnaire_upload should have resulted in a successful form submission. Also, questionnaire is removed from the default post_data in order to simulate the missing questionnaire fields in the template if the project has records.

When should this PR be merged

Soon.

Risks

None foreseen.

Follow up actions

None.

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.

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

Your proposed solution solves only half the problem. Initially, I implemented this that way because I want to make sure, you can't change the questionnaire when there is data in the project (though my original solution is incredibly shortsighted). The reason I wanted to block changing questionnaires not only by hiding the form field in the template but also in the view was that I wanted to have several layers where this is checked and rejected.

I would suggest adding a check to the ProjectEditDetails form; for instance into the clean_questionnaire to raise a validation error. I won't have any effect on what it displayed to the user, but we will be on the safe side in case the form is reused at some point. I can't implement that and add it to your PR if you want (after all it's me who caused the issue).

oliverroick · 2016-12-22T13:55:23Z

cadasta/organization/views/default.py

@@ -652,7 +652,7 @@ def get_initial(self):

    def post(self, *args, **kwargs):
        if self.get_project().has_records:


The if condition is not necessary anymore. So I would get rid of it.

Maybe there can still be a use for this condition except that the super().post() should be outside. Right now, neither the form nor the view checks that the user has not attempted to update the questionnaire when there are already records.

So maybe the condition should be moved to the form and the form (save()) checks that the questionnaire is unchanged or is not provided in the POST data when the project has records.

oliverroick · 2016-12-22T13:58:55Z

cadasta/organization/tests/test_views_default_projects.py

@@ -934,7 +933,7 @@ class ProjectEditDetailsTest(ViewTestCase, UserTestCase,
    }

    def setup_models(self):
-        self.project = ProjectFactory.create(current_questionnaire='abc')


I don't agree with this. We should still test that the view does not update the questionnaire if the project has data. So we should write a test that rejects the update, even though with the current template it's not possible to upload a new questionnaire. My rationale behind it is that if we change the template for some reason and accidentally enable to update the questionnaire, the update will still be rejected from the view, no matter hat the template looks like.

This makes sense. But as I mentioned in another comment, neither the form nor the view does any rejection. So we would need to add that rejection functionality before we can test it.

BTW, it seems that the original line of code (ProjectFactory.create(current_questionnaire='abc')) does not actually create a project with a valid questionnaire. We would need to create a Questionnaire instance too in setup_models.

seav · 2016-12-22T14:20:57Z

Another code observation. Please refer to the following lines of code: https://github.com/Cadasta/cadasta-platform/blob/bugfix/%231018/cadasta/organization/forms.py#L353-L361

Given that the current template does not include the questionnaire form fields when there are records, new_form will be false. This would mean that self.instance.current_questionnaire = '' would be executed which implies that the questionnaire was removed. I think these lines of code need to be rethought.

oliverroick · 2016-12-22T15:46:45Z

I added necessary checks to the form also taking into account Eugene's remarks. Tests have both added to views and forms covering various scenarios. Adding @seav as reviewer for my changes.

seav

I have left some change requests and comments.

seav · 2016-12-23T12:28:36Z

cadasta/organization/forms.py

@@ -357,7 +368,7 @@ def save(self, *args, **kwargs):
                    original_file=original_file,
                    project=self.instance
                )
-        else:
+        elif not self.instance.has_records:


Is this condition really correct? This condition will be evaluated only if new_form is false which can be due to two things:

There was an existing questionnaire but it was removed or it remained empty (questionnaire == '')

The field was missing which implies that the questionnaire should remain unchanged (questionnaire == None).

Now the condition checks whether the project already has records and then clears out current_questionnaire when there are no records. I can only consider scenario 1 above to be valid in this case. For scenario 2, current_questionnaire should remain unchanged. I suggest adding an additional condition to check that new_form is not None.

seav · 2016-12-23T12:41:34Z

cadasta/organization/tests/test_views_default_projects.py

@@ -933,7 +934,7 @@ class ProjectEditDetailsTest(ViewTestCase, UserTestCase,
    }

    def setup_models(self):
-        self.project = ProjectFactory.create()
+        self.project = ProjectFactory.create(current_questionnaire='abc')


Take note that this project creation does not actually create any valid Questionnaire instances. This means that when the view's get_initial() method is executed, Questionnaire.DoesNotExist will be thrown and initial['questionnaire'] will never be populated. This may have an effect later on when the form is submitted and we are comparing new_form with current_form.

seav · 2016-12-23T12:50:32Z

cadasta/organization/tests/test_views_default_projects.py

+        SpatialUnitFactory.create(project=self.project)
+        user = UserFactory.create()
+        assign_policies(user)
+        post_data = self.post_data.copy()


You copied post_data here then deleted the questionnaire entry in the next line. But you never actually used this modified post_data in the request. Based on how I understand django-skivvy to work, the call to request() later on will still use the original self.post_data that still contains the questionnaire entry.

If I understand the test intent correctly, the idea is to try and submit a POST request which does not include the questionnaire field (questionnaire == None). This implies that the questionnaire should remain unchanged which is a valid scenario. Therefore the test should have checked for a 302 status with updated project details, not 200 and unchanged project details.

oliverroick · 2017-01-02T11:44:11Z

Good points @seav. I addressed the issues in the latest commit.

seav

These latest changes are OK with me. The ball is now in @bjohare's court.

linzjax

Everything seems to be working.

seav requested review from oliverroick and bjohare December 22, 2016 13:38

oliverroick requested changes Dec 22, 2016

View reviewed changes

seav commented Dec 23, 2016

View reviewed changes

seav commented Jan 3, 2017

View reviewed changes

seav force-pushed the bugfix/#1018 branch from 3b6e66a to 0e1478d Compare January 3, 2017 05:43

oliverroick added PR: bugfix priority: high PR: needs review labels Jan 3, 2017

oliverroick requested review from linzjax and removed request for bjohare January 3, 2017 15:54

oliverroick force-pushed the bugfix/#1018 branch from 0e1478d to 136b191 Compare January 3, 2017 15:56

oliverroick approved these changes Jan 3, 2017

View reviewed changes

linzjax approved these changes Jan 3, 2017

View reviewed changes

oliverroick added PR: approved and removed PR: needs review labels Jan 3, 2017

amplifi added PR: needs testing and removed PR: needs testing labels Jan 6, 2017

seav and others added 3 commits January 10, 2017 00:10

Fix #1018: Make project details editing work again

2ad74ae

Add checks to form

8ffd278

Addressing Egene's review requests

8db3b32

amplifi force-pushed the bugfix/#1018 branch from 136b191 to 8db3b32 Compare January 9, 2017 16:10

amplifi merged commit 28cf61e into master Jan 9, 2017

amplifi deleted the bugfix/#1018 branch January 9, 2017 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1018: Make project details editing work again #1019

Fix #1018: Make project details editing work again #1019

seav commented Dec 22, 2016 •

edited by linzjax

Loading

oliverroick left a comment

oliverroick Dec 22, 2016

seav Dec 22, 2016

oliverroick Dec 22, 2016

seav Dec 22, 2016

seav Dec 22, 2016

seav commented Dec 22, 2016 •

edited

Loading

oliverroick commented Dec 22, 2016 •

edited

Loading

seav left a comment

seav Dec 23, 2016

seav Dec 23, 2016

seav Dec 23, 2016

oliverroick commented Jan 2, 2017

seav left a comment

linzjax left a comment

		@@ -652,7 +652,7 @@ def get_initial(self):

		def post(self, args, *kwargs):
		if self.get_project().has_records:

Fix #1018: Make project details editing work again #1019

Fix #1018: Make project details editing work again #1019

Conversation

seav commented Dec 22, 2016 • edited by linzjax Loading

Proposed changes in this pull request

When should this PR be merged

Risks

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

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

seav commented Dec 22, 2016 • edited Loading

oliverroick commented Dec 22, 2016 • edited Loading

seav 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

oliverroick commented Jan 2, 2017

seav left a comment

Choose a reason for hiding this comment

linzjax left a comment

Choose a reason for hiding this comment

seav commented Dec 22, 2016 •

edited by linzjax

Loading

seav commented Dec 22, 2016 •

edited

Loading

oliverroick commented Dec 22, 2016 •

edited

Loading