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

Heading length error messaging #97

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Heading length error messaging #97

merged 4 commits into from
Dec 11, 2024

Conversation

jtmst
Copy link
Collaborator

@jtmst jtmst commented Dec 10, 2024

Original issue: #25

This adds more specific error messaging for cases where validation errors are thrown for long headings. New text:

Screenshots:
image

QA Steps:

  1. Navigate to http://localhost:8000/nofos/import
  2. Import a NOFO that has a heading that exceedes the character limit of 511
  3. Verify that error message message renders as expected with a status code of 422

@jtmst jtmst requested a review from pcraig3 December 10, 2024 16:10
@jtmst jtmst changed the title Heading error messaging Heading length error messaging Dec 10, 2024
Copy link
Collaborator

@pcraig3 pcraig3 left a comment

Choose a reason for hiding this comment

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

This looks good!

I have 2 requested changes for you.

make the app less fun

The first is that I am using cool uk slang words when errors come up, but we should 100% definitely stop doing this:

Easiest thing is just remove those words (and the "dang/sorry", etc) and keep things professional.

I know that this is not part of the issue description, but it should be a quick fix to modify those templates.

better signposting for this specific issue

I feel like we should just white-glove this issue.

When you import a NOFO that fails to validate, it actually gets partially created. The subsection that has the title that is too long fails to be created but the one beforehand is created.

At this point we know 3 things:

  • the NOFO
  • the last compliant subsection
  • the user who uploaded it

So let's:

  • link to the NOFO (the latest one we have)
  • pull in the title of the last subsection in the NOFO (also the most recently created subsection, most likely)
  • change the group to be the user's group

By default, all NOFOs are given the "bloom" group, but we should actually override that here and add the user's group. The reason for this is that if a HRSA user runs into this error, their NOFO is not actually a HRSA NOFO yet, so they can't see it even if we link them to it. But they should be able to take a look even though it is busted.

So we should add another line that says something like:

Your document ("title of nofo" + link) contains a heading that is too long. Headings have a character limit of 511 characters.

This usually means that a paragraph has been incorrectly styled as a heading. 
The last valid heading was "name of heading" (no link), so the incorrectly tagged paragraph is most likely after this heading."

Also, I know this is bad practice, but can you leave a TODO in the codebase that says something like # TODO: dynamically pull in heading character limit from models.py _or_ error message.

We should do it, but since we are going to be updating the schema soon, we can wait until then.

@jtmst jtmst requested a review from pcraig3 December 11, 2024 15:34
Copy link
Collaborator

@pcraig3 pcraig3 left a comment

Choose a reason for hiding this comment

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

Almost there! Just a couple things:

1. Set nofo.group equal to user's group

If you create a new user for yourself that's not "Bloomworks" (I have a fake HRSA user that I log in as locally sometimes), and try this, you will get a "Forbidden" when you try to click on the link.

Screenshot 2024-12-11 at 10 33 07 AM

The fix is that we set the nofo.group equal to the user.group. It's not very DRY of us, but we can let this slide.

2. Add the nofo section name also

The message looks good, but since we're already most of the way there, let's extend the message to include the subsection name and the section name.

So, the before and after would look like:

Before:

The last valid heading was "Statutory authority", so the incorrectly tagged paragraph is most likely after this heading.

After:

The last valid heading was "Statutory authority" in "Step 1: Review the Opportunity", so the incorrectly tagged paragraph is most likely after this heading.

The way NOFOs are set up, there is guaranteed to be a section name if there is a subsection name.

@jtmst jtmst requested a review from pcraig3 December 11, 2024 19:56
Copy link
Collaborator

@pcraig3 pcraig3 left a comment

Choose a reason for hiding this comment

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

Looks great! Way better than before.

Feel free to rebase and merge!

Screenshot 2024-12-11 at 4 01 27 PM

@jtmst jtmst merged commit fad322b into main Dec 11, 2024
4 checks passed
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