Improvements to AMY docs and UI #2337
Conversation
elichad
requested review from
pbanaszkiewicz and
maneesha
and removed request for
maneesha
amy/workshops/forms.py
Outdated
| raise forms.ValidationError( | ||
| "You must add tags corresponding to these curricula." | ||
| ) | ||
| missing_tags.add(tag) |
There was a problem hiding this comment.
Alternatively:
missing_tags = expected_tags - set(tags.values_list('name', flat=True))
amy/workshops/forms.py
Outdated
| if missing_tags: | ||
| raise forms.ValidationError( | ||
| f"You must add tags corresponding to these curricula. Missing tags: {', '.join(missing_tags)}" | ||
| ) |
There was a problem hiding this comment.
So this method clean_curricula validates tags based on selected curricula. I think it should be clean_tags, so that at least the error shows up in proper place.
Also it could be beneficial for user experience if the fields in the form switched places.
There was a problem hiding this comment.
I agree - though updating this was less straightforward than I thought. In short, curricula wasn't in cleaned_data at the time clean_tags was run during validation, so I've changed this method to be called during clean() - following these Django docs https://docs.djangoproject.com/en/4.1/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other. Can you do a quick re-review?
There was a problem hiding this comment.
@elichad Looks okay except for the way validation error is handled.
We have this pattern:
errors = {}
if validation_failed_for_a_field:
errors["field"] = ValidationError("Text")
# ...
if errors:
raise ValidationError(errors)For example: CommunityRoleForm.clean, amy/communityroles/forms.py.
There was a problem hiding this comment.
Ah, thanks for the reference, done
amy/workshops/forms.py
Outdated
| @@ -580,7 +580,8 @@ def clean_curricula(self): | |||
| missing_tags.add(tag) | |||
| if missing_tags: | |||
| raise forms.ValidationError( | |||
| f"You must add tags corresponding to these curricula. Missing tags: {', '.join(missing_tags)}" | |||
| f"""You must add tags corresponding to these curricula. """ | |||
| f"""Missing tags: {', '.join(missing_tags)}""" | |||
There was a problem hiding this comment.
In lines 583 and 584 you don't need to use """. You can use " just fine.
Additionally, in L583 you defined an f-string, which is not used and will highlight in some linters. Drop f and you should be fine.
| if not cleaned_data: | ||
| return cleaned_data | ||
|
|
||
| errors: defaultdict[str, list[ValidationError]] = defaultdict(list) |
elichad
deleted the
feature/2328-Comments-from-review-of-AMY-docs-and-corresponding-UI-operations
branch
Fixes some items in #2328, fixes #2331