Skip to content

Refactor Alert partial to ViewComponent#5565

Merged
zachmargolis merged 11 commits intomainfrom
aduth-alert-component
Nov 2, 2021
Merged

Refactor Alert partial to ViewComponent#5565
zachmargolis merged 11 commits intomainfrom
aduth-alert-component

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 29, 2021

Why: Consistency, for all the benefits of using ViewComponent in the first place, and to avoid further entrenching ourselves into the partial with ongoing work including alerts (e.g. vendor outages handling).

Easier review with whitespace hidden: ?w=1

**Why**: Consistency, for all the benefits of using ViewComponent in the first place, and to avoid further entrenching ourselves into the partial with ongoing work including alerts (e.g. vendor outages handling).
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

@aduth aduth marked this pull request as draft November 1, 2021 19:51
@aduth aduth marked this pull request as ready for review November 2, 2021 14:56
@aduth
Copy link
Contributor Author

aduth commented Nov 2, 2021

message.html_safe :[ :[ is that because the message gets serialized into the flash and loses its notion of HTML safeness?

It existed previously, but yeah, I don't love it either. I don't recall off the top of my head what it's needed for, but I'll dig a bit to see if we need it or could remove it.

@aduth
Copy link
Contributor Author

aduth commented Nov 2, 2021

message.html_safe :[ :[ is that because the message gets serialized into the flash and loses its notion of HTML safeness?

So, we do still need it in some circumstances, such as the IAL2 "review" screen:

Screen Shot 2021-11-02 at 4 25 30 PM

flash_now[:success] = flash_message_content

I think the reason is related to the caveat mentioned in the Rails i18n documentation:

Automatic conversion to HTML safe translate text is only available from the translate view helper method.

Because the translation is happening in the controller, I suppose it's not automatically marked as safe.

That being said, it seems like the sort of thing we ought to opt-in to, by explicitly calling .html_safe on the translation result.

They're a bit tricky to track down, though. I tried doing some regex searches like flash(_now)?\[(.|\n)+?_html, or looking for HTML in notices/en.yml and errors/en.yml, but that doesn't seem particularly exhaustive. Aside from trusting our tests, do you have any other ideas for how we might track these down?

@zachmargolis
Copy link
Contributor

They're a bit tricky to track down, though. I tried doing some regex searches like flash(_now)?\[(.|\n)+?_html, or looking for HTML in notices/en.yml and errors/en.yml, but that doesn't seem particularly exhaustive. Aside from trusting our tests, do you have any other ideas for how we might track these down?

We could add a wrapper around flash assignment that checks for < inside of the strings we set in the flash and log a message with the source line, and deploy it for a week or two and see what turns up?

But either way, like you said it was already there and I think the html_safe might be worth filing a followup ticket for

@zachmargolis
Copy link
Contributor

admin merging to get around codeclimate issue

@zachmargolis zachmargolis merged commit 759408a into main Nov 2, 2021
@zachmargolis zachmargolis deleted the aduth-alert-component branch November 2, 2021 20:47
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.

2 participants