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

Fix countdown JavaScript error for lockout screens #6129

Merged
merged 11 commits into from
Apr 4, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 31, 2022

Regression of #6023

Why: So we don't have any errors, and so that the countdown counts down as expected.

NR: https://onenr.io/0BQ1AyEdAQx

Draft: Need to resolve issue where scripts associated with the view component aren't being printed. Initial degugging shows that view_context passed to presenter is distinct from rendering template, thus maybe not printing scripts added through associated script helper (see similar issue). Edit: In dcef566, view context is passed directly from view. See related comment at #6129 (comment).

**Why**: So we don't have any errors, and so that the countdown counts down as expected.
because it's different instance than when presenter is constructed in controller, so calling to enqueue scripts must be on same instance b/w component render and view render
@@ -6,7 +6,7 @@
<%= render PageHeadingComponent.new.with_content(presenter.header) %>
<% end %>

<% Array(presenter.description).each do |description_p| %>
<% Array(presenter.description(self)).each do |description_p| %>
Copy link
Member Author

@aduth aduth Mar 31, 2022

Choose a reason for hiding this comment

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

I don't like it, and we encountered it once before in #6023 (comment), but as noted in extended commit description of dcef566 and original pull request comment text, we can't rely on view_context remaining constant between the time of presenter initialization and view rendering (source, related issue). The value of view_context in a constructed presenter may receive scripts to enqueue associated with the component, but those scripts would not be the same as rendered into the template. We sorta avoided this in #6023 because the presenter was constructed inside a helper, so view rendering had already begun, and the render-bound view_context was available.

I think an ideal long-term solution is to remove these presenters and views altogether, then create a shared "Failure" component that can take the place of this view and the idv/shared/_error.html.erb partial, then each failure type can have a dedicated view that calls directly to the component.

e.g.

<%# app/views/shared/_otp_max_attempts %>

<%= render ErrorPageComponent.new(...) %>

@aduth aduth marked this pull request as ready for review March 31, 2022 20:42
@aduth aduth merged commit 48a6948 into main Apr 4, 2022
@aduth aduth deleted the aduth-replace-countdown-ref branch April 4, 2022 12:23
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.

3 participants