-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add soft validation of member codes to training request update form #2560
Add soft validation of member codes to training request update form #2560
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.
Looks good, but I strongly suggest moving member_code_valid
to exception-based approach.
@@ -2255,7 +2255,7 @@ class TrainingRequest( | |||
nonprofit_teaching_experience = models.CharField( | |||
max_length=STR_LONGEST, | |||
blank=True, | |||
null=True, | |||
default="", |
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.
Thanks for these changes.
|
||
class Migration(migrations.Migration): | ||
|
||
replaces = [('workshops', '0269_alter_trainingrequest_nonprofit_teaching_experience'), ('workshops', '0270_auto_20231030_0919')] |
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.
What causes this? Can you use a rebased codebase or alter migration 0269_alter_trainingrequest_nonprofit_teaching_experience
(it's okay before we release to prod, and even safer if it wasn't released on test-amy)?
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.
I squashed two uncommitted migrations to make this one, and this line resulted from that. I'll delete it.
amy/extrequests/forms.py
Outdated
# find relevant membership - may raise Membership.DoesNotExist | ||
membership = Membership.objects.get(registration_code=code) | ||
except Membership.DoesNotExist: | ||
return False, f'No membership found for code "{code}".' |
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.
You could use custom exceptions instead of returning a flag and text.
For example:
class MemberCodeValidationError(ValueError): # or inherit from ValidationError
pass
...
def member_code_valid(self, code: str) -> bool:
"""Returns True if the code matches an active Membership with available seats,
including a grace period of 90 days before and after the Membership dates.
If there is no match, throws an exception with detailed error.
"""
try:
# find relevant membership - may raise Membership.DoesNotExist
membership = Membership.objects.get(registration_code=code)
except Membership.DoesNotExist as exc:
raise MemberCodeValidationError(f'No membership found for code "{code}".') from exc
...
return True
amy/extrequests/forms.py
Outdated
|
||
return errors | ||
|
||
def member_code_valid(self, code: str) -> tuple[bool, str]: |
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.
I'd consider making this a function receiving code
and timestamp
(to be used instead of self.instance.created_at
) and moving outside the class - as a util function. I'd also add unit tests for it.
Fixes #2550.
The implementation is very similar to #2549, but allows the actual reason for failure to be shown (rather than the list of possible reasons).
While testing I discovered some
CharField
andTextField
fields onTrainingRequest
which hadblank=True, null=True
, so I changed these to havedefault=""
instead in line with best practices.