-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/session question unique key work around #119
Feature/session question unique key work around #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @moh-moola
I've just got some questions
molo/surveys/models.py
Outdated
@@ -82,8 +82,11 @@ class Meta: | |||
|
|||
@property | |||
def clean_name(self): | |||
return str(slugify(text_type(unidecode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we remove this whole method? Since you added it in your original PR and the base class does it like this?
molo/surveys/utils.py
Outdated
self.new_answers.update({ | ||
checkbox.clean_name: 'off' | ||
for checkbox in self.pk_cleaned_missing_checkboxes | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moh-moola Doesn't the above code mean that we're adding the missing checkboxes twice? Once with their clean_name and once with their pk_clean_name
And how does this affect checkboxes that were answered? because wouldn't that exist with their clean_name but not with their pk_clean_name and so we'll add conflicting information? Or will they get added before this?
molo/surveys/forms.py
Outdated
@@ -20,6 +20,17 @@ | |||
CHARACTER_COUNT_CHOICE_LIMIT = 512 | |||
|
|||
|
|||
class SurveyBaseForm(BaseForm): | |||
|
|||
def pk_cleaned_data(self, fields): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we be able to put this in the survey model? We could use the get_fields
method and pass in the cleaned_data
That way you wouldn't have to overwrite the form class. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KaitCrawford I am not sure how you mean by this, but I have since tried to clean up the code
@KaitCrawford Please review, I have implemented your suggested changes and all seems to be well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.