Skip to content
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

LGA-956 Error Messages #942

Merged
merged 49 commits into from
Jul 9, 2020
Merged

LGA-956 Error Messages #942

merged 49 commits into from
Jul 9, 2020

Conversation

MalcolmVonMoJ
Copy link
Contributor

@MalcolmVonMoJ MalcolmVonMoJ commented May 11, 2020

What does this pull request do?

  • Added specific error messages for each question, removing the format where the question text was repeated with a generic error.
  • Rephrased other error messages.
  • New error messages for currency rate question types - errors for no value and no frequency, distinct from each other. (New variables for this: freq_message when amount specified but rate (e.g. per week) not answered, and amount_message for the opposite. This has been added to the MoneyIntervalAmountRequired function in validators.py.)
  • Stopped error styling from cascading - error message being present still cascades as before.
  • Error summary re-positioned at the top of the page - JS change and structure change on many pages - this includes the {{ Form.handle_errors(form) }} being re-positioned on about 10 pages.
  • Error summary changed to not use the dl dt dd structure - as per design system.
  • Added new JS function (in base.html) for error summary links so they focus on the errorred answer box but only scrolls as far as the relevant question text.
  • Changed the error state code so child questions are not in error state if their parent questions are null - affected pop-up questions when their parent was unanswered, errorred, and subsequently answered. (This might have been raised as another bug, but I can't find it now.)
  • Added Welsh language for new wording.
  • Some smart quote correction and white space tweaks.
  • New highlighting for error-state answer boxes.
  • Errors re-positioned in relation to the question and hint text (in form.html).

Any other changes that would benefit highlighting?

  • Some smart quote correction.

Checklist

  • Provided JIRA ticket number in the title, e.g. "LGA-152: Sample title"

@MalcolmVonMoJ MalcolmVonMoJ requested a review from a team as a code owner May 11, 2020 10:44
@MalcolmVonMoJ MalcolmVonMoJ changed the title Feature/lga 956 errors [BLOCKED] LGA-956 Error Messages May 11, 2020
@MalcolmVonMoJ MalcolmVonMoJ force-pushed the feature/LGA-956-errors branch 7 times, most recently from 7e0beca to 30902e8 Compare May 14, 2020 07:18
@MalcolmVonMoJ MalcolmVonMoJ force-pushed the feature/LGA-956-errors branch 2 times, most recently from 9d734b3 to 4866ab5 Compare June 24, 2020 14:05
@MalcolmVonMoJ MalcolmVonMoJ force-pushed the feature/LGA-956-errors branch 3 times, most recently from 495a9bb to dad6d38 Compare July 1, 2020 09:53
@MalcolmVonMoJ MalcolmVonMoJ changed the title [BLOCKED] LGA-956 Error Messages LGA-956 Error Messages Jul 3, 2020
Copy link
Contributor

@exonian exonian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some excellent stuff here.

One potential missed thing (or otherwise something needs explaining for the likes of me! 😅) and a method which has now become rather complex and hard to read that I'd like us to refactor.

cla_public/apps/checker/forms.py Show resolved Hide resolved
cla_public/apps/checker/validators.py Outdated Show resolved Hide resolved
@@ -83,7 +83,7 @@ def __init__(self, message=None, freq_message=None, amount_message=None):
self.freq_message = freq_message
self.amount_message = amount_message

def append_interval_text(self, field):
def append_interval_text(self, field, interval):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I spotted this when we were writing the signature but planned to lead you towards finding it yourself and then completely forgot 🤦 😅

self.amount_message = amount_message

def append_interval_text(self, field, interval):
if interval.data == "per_week":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection, I think this might be worth refactoring to avoid the multiple ifs. Do you fancy pairing on it? Think it could be quite enjoyable.

Copy link
Contributor

@exonian exonian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy with this

@MalcolmVonMoJ MalcolmVonMoJ merged commit 6a605ff into master Jul 9, 2020
@MalcolmVonMoJ MalcolmVonMoJ deleted the feature/LGA-956-errors branch July 9, 2020 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants