-
Notifications
You must be signed in to change notification settings - Fork 6
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
♿ [#2374] Add more verbose error-text for input fields #1285
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1285 +/- ##
===========================================
- Coverage 44.26% 44.22% -0.05%
===========================================
Files 972 972
Lines 35414 35602 +188
===========================================
+ Hits 15677 15744 +67
- Misses 19737 19858 +121 ☔ View full report in Codecov by Sentry. |
64228e1
to
30f1901
Compare
f72759b
to
c590d27
Compare
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.
The failing test is to be expected, since you're mixing in the newly defined ErrorMessageMixin
into UserForm
. Update the assertion in the test to reflect the new message.
email = forms.EmailField(required=True, label="E-mailadres") | ||
|
||
|
||
class ErrorMessageMixinTests(TestCase): |
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.
Let's stick to singular nouns for classes. There are many tests in ErrorMessageMixinTest
, but there is only one class. I know our code is not consistent, but we should try.
@@ -40,6 +41,24 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class ErrorMessageMixin(FormMixin): |
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 agree with your comment that displaying the error message twice is overkill, so this class can be removed.
src/open_inwoner/utils/forms.py
Outdated
} | ||
|
||
def __init__(self, *args, **kwargs): | ||
user_added_messages = {} |
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.
This should be simply custom_error_messages
, it contrasts with the class-level default_error_messages
. This way the code is consistent with the test you wrote.
LoginRequiredMixin, | ||
CommonPageMixin, | ||
BaseBreadcrumbMixin, | ||
ErrorMessageMixin, |
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.
Not needed if the above class is removed
issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2374
This one is more for cognitive disabilities, where error notifications need to be more clear in the actions they expect from the user.
Would be good to have this in as many forms as possible and not just the 1 emailfield in the profiel-edit page.