HEEDLS-643 Fix character limit error on admin fields and registration… #684
Conversation
There was a problem hiding this comment.
I think it would be good to try and add unit tests for both these methods so this issue doesn't crop up again in the future, although I'm not sure how simple that would be. It should be possible to copy the setup for AddRegistrationPromptConfigureAnswers_add_configures_new_answer_and_updates_temp_data, but change the assertions to check the modelstate errors of the view result
...lutions.Web/Controllers/TrackingSystem/Centre/Configuration/RegistrationPromptsController.cs
Show resolved
Hide resolved
livzorn
force-pushed
the
HEEDLS-643-prompt-answer-character-limit
branch
from
77a8261 to
185c91a
Compare
|
|
||
| var remainingLength = 1000 - (model.OptionsString?.Length - 2 ?? 0); | ||
| var remainingLength = 1000 - model.OptionsString.Length; | ||
| var remainingLengthShownToUser = remainingLength <= 2 ? 0 : remainingLength - 2; |
There was a problem hiding this comment.
Do we think it is clear enough to just show zero if the answer is over the limit?
I'm wondering about showing how many characters are over the limit so the user knows how much they need to shorten their answer by. But then that might introduce confusion if they then want to add another answer which will mysteriously be another 2 characters shorter.
Do we think its not worth the bother?
There was a problem hiding this comment.
Hmm right. I think it would be useful to know how many characters you've entered so you can compare it against the number remaining, and I think it would be more useful than confusing. This error will probably not be encountered very often, and I think it would be more useful than confusing. Added in the number of characters entered to the error message.
There was a problem hiding this comment.
Just need to fix the tests accordingly, will do after lunch!
There was a problem hiding this comment.
Tests are passing now, and the new error message looks like this, let me know what you think:
There was a problem hiding this comment.
I think this looks pretty good. Minor suggestion, I think it would be better if it said "x characters were entered" to make it slightly clearer that those characters are the ones the user just tried to enter. Does that sound reasonable?
| ModelState.AddModelError( | ||
| nameof(AdminFieldAnswersViewModel.Answer), | ||
| $"The complete list of answers must be 1000 characters or fewer ({remainingLength} characters remaining for the new answer)" | ||
| $"The complete list of answers must be 1000 characters or fewer ({remainingLengthShownToUser} characters remaining for the new answer)" |
There was a problem hiding this comment.
Could we make the "characters" after the {remainingLengthShownToUser} to be singular if there is only 1 character remaining (I think we have a DisplayStringHelper with a method in for adding an "s" to a string if a count is not 1).
livzorn
force-pushed
the
HEEDLS-643-prompt-answer-character-limit
branch
from
185c91a to
ba2b0de
Compare
|
|
||
| var remainingLength = 1000 - (model.OptionsString?.Length - 2 ?? 0); | ||
| var remainingLength = 1000 - model.OptionsString.Length; | ||
| var remainingLengthShownToUser = remainingLength <= 2 ? 0 : remainingLength - 2; |
There was a problem hiding this comment.
I think this looks pretty good. Minor suggestion, I think it would be better if it said "x characters were entered" to make it slightly clearer that those characters are the ones the user just tried to enter. Does that sound reasonable?
JIRA link
HEEDLS-643
Description
Adjusted the way the remaining length for prompt answers is calculated, so that the number of characters remaining shown to the user accounts for the "\r\n" that will be added to their answer. If there are 2 or fewer characters remaining, the user will see "0 characters remaining" because it is impossible to submit an answer". If there are more than 2 characters remaining, the error will indicate 2 fewer characters than the actual number of character remaining to allow for the newline separator.
Also fixed the titles on Edit course admin field and Add course admin field pages so their will have an "Error" in them if an error has occurred.
Screenshots
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have: