Conversation
There was a problem hiding this comment.
@hursey013 This is what I ended up doing to eliminate the Rubocop offenses:
- move the flash message text to the existing session decorators
- eliminate the need for methods like
sp_metadataandsp_nameinApplicationControllerby reading the SP attributes from the DB viacurrent_spanddecorated_session, which fetch the SP from the DB using theissuerprovided by eithersp_sessionorparams.
app/services/decorated_session.rb
Outdated
There was a problem hiding this comment.
This is needed because ServiceProvider.from_issuer returns NullServiceProvider if no DB matches were found. From a session decorator standpoint, we want to treat a NullServiceProvider the same as the absence of an SP.
app/models/null_service_provider.rb
Outdated
There was a problem hiding this comment.
maybe class.to_s in case the class ever gets renamed?
There was a problem hiding this comment.
Maybe just leave it nil to avoid confusion? We don't need a string. I wasn't trying to name it the same as the class.
jessieay
left a comment
jessieay
left a comment
There was a problem hiding this comment.
👏 this is SO much better than what we have before
There was a problem hiding this comment.
this might be for a separate PR, but I wonder if it would be better to have an @sp or @current_sp instance var instead of all 3 of these, but to have this info available from that instance var. In general, having more than a couple of instance vars available in any one view is a 🔴 flag to me.
There was a problem hiding this comment.
Yep. Great minds thing alike. I was going to create a view object in a separate PR.
There was a problem hiding this comment.
can we get sp_name from current_sp instead?
There was a problem hiding this comment.
I thought about that, but that would mean creating a sp_name method in the ServiceProvider model, which goes against our rule of keeping business logic, and especially view-only logic out of models. What we could do is create a ServiceProviderDecorator that defines the logo, name, and return_to_sp_url.
app/decorators/session_decorator.rb
Outdated
There was a problem hiding this comment.
might prefer def sp_name; end for consistency with other methods that return nil in this class
**Why**: Now that Service Providers are stored in the DB, along with all of their attributes, we can read the attributes directly from the DB, as opposed to storing them in the session.
monfresh
force-pushed
the
mb-refactor-sp-session
branch
from
5bdbd1c to
19d38ab
Compare
| end | ||
|
|
||
| def requested_attributes | ||
| OpenidConnectAttributeScoper.new(scopes).requested_attributes |
There was a problem hiding this comment.
this line is the only thing this class does anymore, should we just remove the class entirely and write a @requested_attributes var directly in the controller?
There was a problem hiding this comment.
Agreed. Can we do that in a follow-up PR?
**Why**: Now that Service Providers are stored in the DB, along with all of their attributes, we can read the attributes directly from the DB, as opposed to storing them in the session.
**Why**: Now that Service Providers are stored in the DB, along with all of their attributes, we can read the attributes directly from the DB, as opposed to storing them in the session.
5 participants
Why: Now that Service Providers are stored in the DB, along
with all of their attributes, we can read the attributes directly
from the DB, as opposed to storing them in the session.