LG-15681: Add service provider or logo to ready to verify email #12018
LG-15681: Add service provider or logo to ready to verify email #12018eileen-nava merged 9 commits intomainfrom
Conversation
…ice provider logo or name in ready to verify email
eileen-nava
requested review from
a team,
gina-yamada and
kellular
and removed request for
a team
| @@ -33,6 +33,10 @@ def sp_logo_url | |||
| end | |||
| end | |||
There was a problem hiding this comment.
I think we should consider moving sp_logo_url and friends into ServiceProvider. Would that allow us to avoid passing around the service_provider_session and use enrollment.service_provider.sp_logo_url instead?
There was a problem hiding this comment.
@mitchellhenke That proposed change would enable us to stop passing around the service_provider_session and would enable us to use enrollment.service_provider.sp_logo_url, which would be good. I am wondering how removing sp_logo_url from ServiceProviderSession would affect other areas of the code base. app/views/shared/_nav_branded.html.erb relies on decorated_sp_session to render the service provider logo. Thoughts?
There was a problem hiding this comment.
It would probably require some refactoring of other areas, yeah, and maybe the refactor deserves its own PR, but I don't feel strongly either way. Also happy to help with that, just let me know.
app/mailers/user_mailer.rb
Outdated
| @logo_is_png = logo_is_png | ||
| @sp_logo_url = sp_logo_url |
There was a problem hiding this comment.
If we're not going to use the sp_logo_url in some cases, could we do something like the following and use whether the sp_logo_url is defined in the template?
| @logo_is_png = logo_is_png | |
| @sp_logo_url = sp_logo_url | |
| if logo_is_png | |
| @sp_logo_url = sp_logo_url | |
| else | |
| @sp_logo_url = nil | |
| end |
There was a problem hiding this comment.
I like @mitchellhenke suggestions. I did test current implementation out locally- via mailer and in moving through the flow and it is working as I'd expect. I would like to review code/test if you decide to make his changes. Please reach out so I can review/approve at that time.
…ervice provider logo
shanechesnutt-ft
left a comment
shanechesnutt-ft
left a comment
There was a problem hiding this comment.
Things are looking good. I have a couple more comments that might be nice to also improve.
| let(:view_context) { ActionController::Base.new.view_context } | ||
| let(:sp) { build_stubbed(:service_provider, logo: nil) } | ||
| let(:decorated_sp_session) do | ||
| ServiceProviderSessionCreator.new( | ||
| sp: sp, | ||
| view_context: view_context, | ||
| sp_session: { issuer: sp.issuer }, | ||
| service_provider_request: ServiceProviderRequestProxy.new, | ||
| ).create_session | ||
| end |
There was a problem hiding this comment.
Are the added let variables here still needed (as well as the decorated_sp_session below)?
Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>
shanechesnutt-ft
left a comment
shanechesnutt-ft
left a comment
There was a problem hiding this comment.
Looks good to me! Ran through the test plan another time. 👍🏻
5 participants
🎫 Ticket
LG-15681: Eng: Add Partner Agency logo OR name to barcode emails
🛠 Summary of changes
📜 Testing Plan
Ready to verify email for example OIDC app:
👀 Screenshots
Service provider with a png logo:
Service provider with a svg logo: