Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion cadasta/organization/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,17 @@ class Meta:
fields = ['name', 'description', 'access', 'urls', 'questionnaire',
'contacts']

def clean_questionnaire(self):
new_form = self.data.get('questionnaire')
current_form = self.initial.get('questionnaire')

if (new_form is not None and new_form != current_form and
self.instance.has_records):
raise ValidationError(
_("Data has already been contributed to this project. To "
"ensure data integrity, uploading a new questionnaire is "
"disabled for this project."))

def save(self, *args, **kwargs):
new_form = self.data.get('questionnaire')
original_file = self.data.get('original_file')
Expand All @@ -357,7 +368,7 @@ def save(self, *args, **kwargs):
original_file=original_file,
project=self.instance
)
else:
elif new_form is not None and not self.instance.has_records:
self.instance.current_questionnaire = ''

return super().save(*args, **kwargs)
Expand Down
59 changes: 59 additions & 0 deletions cadasta/organization/tests/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,65 @@ def test_replace_questionnaire(self):
id=project.current_questionnaire
).xls_form.url == data['questionnaire'])

def test_replace_questionnaire_when_project_has_data(self):
project = ProjectFactory.create()
questionnaire = QuestionnaireFactory.create(
project=project,
xls_form=self._get_form('xls-form'))

SpatialUnitFactory.create(project=project)

data = {
'name': 'New name',
'questionnaire': self._get_form('xls-form-copy'),
'access': project.access,
'contacts-TOTAL_FORMS': 1,
'contacts-INITIAL_FORMS': 0,
'contacts-0-name': '',
'contacts-0-email': '',
'contacts-0-tel': ''
}

form = forms.ProjectEditDetails(
instance=project,
data=data,
initial={'questionnaire': questionnaire.xls_form.url})

assert form.is_valid() is False
assert ("Data has already been contributed to this project. To "
"ensure data integrity, uploading a new questionnaire is "
"disabled for this project." in
form.errors.get('questionnaire'))

def test_do_not_send_questionnaire_when_project_has_data(self):
project = ProjectFactory.create()
questionnaire = QuestionnaireFactory.create(
project=project,
xls_form=self._get_form('xls-form'))

SpatialUnitFactory.create(project=project)

data = {
'name': 'New name',
'access': project.access,
'contacts-TOTAL_FORMS': 1,
'contacts-INITIAL_FORMS': 0,
'contacts-0-name': '',
'contacts-0-email': '',
'contacts-0-tel': ''
}

form = forms.ProjectEditDetails(
instance=project,
data=data,
initial={'questionnaire': questionnaire.xls_form.url})

assert form.is_valid() is True
form.save()
project.refresh_from_db()
assert project.name == data['name']
assert project.current_questionnaire == questionnaire.id

def test_delete_questionnaire(self):
project = ProjectFactory.create()
questionnaire = QuestionnaireFactory.create(
Expand Down
37 changes: 36 additions & 1 deletion cadasta/organization/tests/test_views_default_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,8 @@ class ProjectEditDetailsTest(ViewTestCase, UserTestCase,

def setup_models(self):
self.project = ProjectFactory.create(current_questionnaire='abc')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.questionnaire = QuestionnaireFactory.create(project=self.project,
id='abc')

def setup_url_kwargs(self):
return {
Expand All @@ -945,7 +947,11 @@ def setup_url_kwargs(self):
def setup_template_context(self):
return {'project': self.project,
'object': self.project,
'form': forms.ProjectEditDetails(instance=self.project)}
'form': forms.ProjectEditDetails(
instance=self.project,
initial={'questionnaire': self.questionnaire.xls_form.url,
'original_file': self.questionnaire.original_file}
)}

def test_get_with_authorized_user(self):
user = UserFactory.create()
Expand All @@ -956,6 +962,20 @@ def test_get_with_authorized_user(self):
assert response.content == self.expected_content
assert 'Select the questionnaire' in self.expected_content

def test_get_empty_questionnaire_with_authorized_user(self):
user = UserFactory.create()
assign_policies(user)

self.project.current_questionnaire = ''
self.project.save()

form = forms.ProjectEditDetails(instance=self.project)

response = self.request(user=user)
assert response.status_code == 200
assert response.content == self.render_content(form=form)
assert 'Select the questionnaire' in self.expected_content

def test_get_with_blocked_questionnaire_upload(self):
user = UserFactory.create()
assign_policies(user)
Expand Down Expand Up @@ -1010,6 +1030,7 @@ def test_post_with_authorized_user(self):
self.project.refresh_from_db()
assert self.project.name == self.post_data['name']
assert self.project.description == self.post_data['description']
assert self.project.current_questionnaire == ''

def test_post_with_blocked_questionnaire_upload(self):
SpatialUnitFactory.create(project=self.project)
Expand All @@ -1023,6 +1044,20 @@ def test_post_with_blocked_questionnaire_upload(self):
assert self.project.description != self.post_data['description']
assert self.project.current_questionnaire == 'abc'

def test_post_empty_questionnaire_with_blocked_questionnaire_upload(self):
SpatialUnitFactory.create(project=self.project)
user = UserFactory.create()
assign_policies(user)
response = self.request(user=user, method='POST',
post_data={'questionnaire': None})

assert response.status_code == 302
assert self.expected_success_url in response.location
self.project.refresh_from_db()
assert self.project.name == self.post_data['name']
assert self.project.description == self.post_data['description']
assert self.project.current_questionnaire == 'abc'

def test_post_invalid_form(self):
question = self.get_form('xls-form-invalid')
user = UserFactory.create()
Expand Down
17 changes: 7 additions & 10 deletions cadasta/organization/views/default.py
Original file line number Diff line number Diff line change
Expand Up @@ -651,16 +651,13 @@ def get_initial(self):
return initial

def post(self, *args, **kwargs):
if self.get_project().has_records:
return self.get(*args, **kwargs)
else:
try:
return super().post(*args, **kwargs)
except InvalidXLSForm as e:
form = self.get_form()
for err in e.errors:
form.add_error('questionnaire', err)
return self.form_invalid(form)
try:
return super().post(*args, **kwargs)
except InvalidXLSForm as e:
form = self.get_form()
for err in e.errors:
form.add_error('questionnaire', err)
return self.form_invalid(form)


class ProjectEditPermissions(ProjectEdit, generic.UpdateView):
Expand Down