Skip to content

Ensure description if risk_technical #1148

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

Merged
jaredlockhart merged 1 commit into from
Apr 5, 2019
Merged

Ensure description if risk_technical #1148

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
55 changes: 48 additions & 7 deletions app/experimenter/experiments/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,48 +566,68 @@ class RadioWidget(forms.widgets.RadioSelect):
class ExperimentRisksForm(ChangeLogMixin, forms.ModelForm):
RADIO_OPTIONS = ((False, "No"), (True, "Yes"))

def coerce_truthy(value):

This comment was marked as outdated.

Sign in to view

if value.lower() == "true":
return True
elif value.lower() == "false":
return False

# Radio Buttons
risk_internal_only = forms.ChoiceField(
risk_internal_only = forms.TypedChoiceField(
label=Experiment.RISK_INTERNAL_ONLY_LABEL,
help_text=Experiment.RISK_INTERNAL_ONLY_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
Copy link
Copy Markdown
Collaborator

@jaredlockhart jaredlockhart Apr 4, 2019

Choose a reason for hiding this comment

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

I think you can just say coerce=bool here?

Copy link
Copy Markdown
Author

@robhudson robhudson Apr 5, 2019

Choose a reason for hiding this comment

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

The value getting passed to these is a string of either "True" or "False" which come from the RADIO_OPTIONS after going through a string conversion in Django-land. And bool("False") => True which isn't what we want.

empty_value=None,
)
risk_partner_related = forms.ChoiceField(
risk_partner_related = forms.TypedChoiceField(
label=Experiment.RISK_PARTNER_RELATED_LABEL,
help_text=Experiment.RISK_PARTNER_RELATED_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
empty_value=None,
)
risk_brand = forms.ChoiceField(
risk_brand = forms.TypedChoiceField(
label=Experiment.RISK_BRAND_LABEL,
help_text=Experiment.RISK_BRAND_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
empty_value=None,
)
risk_fast_shipped = forms.ChoiceField(
risk_fast_shipped = forms.TypedChoiceField(
label=Experiment.RISK_FAST_SHIPPED_LABEL,
help_text=Experiment.RISK_FAST_SHIPPED_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
empty_value=None,
)
risk_confidential = forms.ChoiceField(
risk_confidential = forms.TypedChoiceField(
label=Experiment.RISK_CONFIDENTIAL_LABEL,
help_text=Experiment.RISK_CONFIDENTIAL_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
empty_value=None,
)
risk_release_population = forms.ChoiceField(
risk_release_population = forms.TypedChoiceField(
label=Experiment.RISK_RELEASE_POPULATION_LABEL,
help_text=Experiment.RISK_RELEASE_POPULATION_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
empty_value=None,
)
risk_technical = forms.ChoiceField(
risk_technical = forms.TypedChoiceField(
label=Experiment.RISK_TECHNICAL_LABEL,
help_text=Experiment.RISK_TECHNICAL_HELP_TEXT,
choices=RADIO_OPTIONS,
widget=RadioWidget,
coerce=coerce_truthy,
empty_value=None,
)

# Optional Risk Descriptions
Expand Down Expand Up @@ -690,6 +710,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 ``True``.
if cleaned_data.get("risk_technical"):
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
59 changes: 44 additions & 15 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 @@ -1041,26 +1041,27 @@ def test_form_saves_objectives(self):

class TestExperimentRisksForm(MockRequestMixin, TestCase):

valid_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": "It's complicated",
"risks": "There are some risks",
"testing": "Always be sure to test!",
"test_builds": "Latest build",
"qa_status": "It ain't easy being green",
}

def test_form_saves_risks(self):
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": "It's complicated",
"risks": "There are some risks",
"testing": "Always be sure to test!",
"test_builds": "Latest build",
"qa_status": "It ain't easy being green",
}

data = self.valid_data.copy()
form = ExperimentRisksForm(
request=self.request, data=data, instance=created_experiment
)
Expand All @@ -1083,6 +1084,34 @@ def test_form_saves_risks(self):
self.assertEqual(experiment.test_builds, data["test_builds"])
self.assertEqual(experiment.qa_status, data["qa_status"])

def test_risk_technical_description_empty(self):
created_experiment = ExperimentFactory.create_with_status(
Experiment.STATUS_DRAFT
)

data = self.valid_data.copy()
data["risk_technical_description"] = ""

form = ExperimentRisksForm(
request=self.request, data=data, instance=created_experiment
)
self.assertFalse(form.is_valid())
self.assertIn("risk_technical_description", form.errors)

def test_risk_technical_description_empty_not_risk_technical(self):
created_experiment = ExperimentFactory.create_with_status(
Experiment.STATUS_DRAFT
)

data = self.valid_data.copy()
data["risk_technical"] = False
data["risk_technical_description"] = ""

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


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