Skip to content

risk_technical and risk_technical_description invariance fixes #1082 #1103

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.

Sign up for GitHub

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

Closed
peterbe wants to merge 1 commit into from
Closed

risk_technical and risk_technical_description invariance fixes #1082 #1103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions app/experimenter/experiments/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,27 @@ class Meta:
"qa_status",
)

def clean(self):
cleaned_data = super().clean()
if (
"risk_technical" in cleaned_data
and "risk_technical_description" in cleaned_data
):
# Both checked, now we need to do an invariance check on these
# two. This is to match what's done with jQuery in the form:
# the 'risk_technical_description' needs to be required
# if 'risk_technical' is truthy.
if cleaned_data.get("risk_technical") == "True":
Copy link
Copy Markdown
Contributor Author

@peterbe peterbe Mar 22, 2019

Choose a reason for hiding this comment

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

It's weird to me that these are strings like that. Even after supposed cleaning. I've never seen that before. Is there a better way to do this?

Copy link
Copy Markdown
Collaborator

@jaredlockhart jaredlockhart Mar 22, 2019

Choose a reason for hiding this comment

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

Yeah that is weird?

Copy link
Copy Markdown

@robhudson robhudson Apr 4, 2019

Choose a reason for hiding this comment

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

It looks like this is because the base form field is a ChoiceField which coerces all values to strings. Since the underlying model for these fields is a boolean we should use TypedChoiceField here and provide a proper coerce function to make this nicer.

if not cleaned_data["risk_technical_description"]:
msg = (
f"This is required if "
f"'{Experiment.RISK_TECHNICAL_LABEL}' is true."
)
raise forms.ValidationError(
{"risk_technical_description": msg}
)
return cleaned_data


class ExperimentReviewForm(
ExperimentConstants, ChangeLogMixin, forms.ModelForm
Expand Down
40 changes: 40 additions & 0 deletions app/experimenter/experiments/tests/test_forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,46 @@ def test_form_saves_risks(self):
self.assertEqual(experiment.test_builds, data["test_builds"])
self.assertEqual(experiment.qa_status, data["qa_status"])

def test_form_risk_technical_invariance(self):
Copy link
Copy Markdown
Collaborator

@jaredlockhart jaredlockhart Mar 22, 2019

Choose a reason for hiding this comment

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

I don't think the word 'invariance' is descriptive here, the issue is that if risk_technical is true then risk_technical_description is required, so maybe something like test_risk_technical_description_optionally_required would be clearer.

created_experiment = ExperimentFactory.create_with_status(
Experiment.STATUS_DRAFT
)

data = {
"risk_internal_only": True,
"risk_partner_related": True,
"risk_brand": True,
"risk_fast_shipped": True,
"risk_confidential": True,
"risk_release_population": True,
"risk_technical": True,
"risk_technical_description": "", # Note!
"risks": "There are some risks",
"testing": "Always be sure to test!",
"test_builds": "Latest build",
"qa_status": "It ain't easy being green",
}

form = ExperimentRisksForm(
request=self.request, data=data, instance=created_experiment
)
self.assertFalse(form.is_valid())

# It would be okey if 'risk_technical' was falsy.
data["risk_technical"] = False
form = ExperimentRisksForm(
request=self.request, data=data, instance=created_experiment
)
self.assertTrue(form.is_valid())

# And definitely OK if both at truthy.
Copy link
Copy Markdown
Collaborator

@jaredlockhart jaredlockhart Mar 22, 2019

Choose a reason for hiding this comment

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

if both *are truthy

data["risk_technical"] = True
data["risk_technical_description"] = "Some text"
form = ExperimentRisksForm(
request=self.request, data=data, instance=created_experiment
)
self.assertTrue(form.is_valid())
Copy link
Copy Markdown
Collaborator

@jaredlockhart jaredlockhart Mar 22, 2019

Choose a reason for hiding this comment

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

I'd rather have each of these cases tested in separate tests, and also shouldn't we be testing the case where risk_technical is true but risk_technical_description is empty and the form fails to validate?



class TestExperimentReviewForm(
MockRequestMixin, MockBugzillaMixin, MockTasksMixin, TestCase
Expand Down