Fix rubocop Lint/DuplicateMethods #12350
Conversation
rioug
added
the
technical changes only
There was a problem hiding this comment.
I think this might actually break things, which are not tested.
But before we proceed any further, I'll try to find out if it's even used...
I've posted in #delivery-circle, so let's pause until we can resolve that question.
| attr_accessor(*ACCESSORS, :sso_secret, :sso_url) | ||
| attr_accessor(*ACCESSORS) | ||
|
|
||
| def self.sso_secret | ||
| raise "sso_secret not implemented on class, be sure to set it on instance" |
There was a problem hiding this comment.
Hmm I don't see the instance method declared elsewhere, so I wonder if it's confused the class method as a duplicate.
Sorry I didn't read the PR description properly, I now understand!
I can't actually find any specs covering this, so I tried constructing a quick test in the console:
require 'discourse/single_sign_on'
sso = Discourse::SingleSignOn.parse("query_string", "secret")
NoMethodError: undefined method `sso_secret=' for #<Discourse::SingleSignOn:0x0000000114c12798>
sso.sso_secret = sso_secret if sso_secret
^^^^^^^^^^^^^
Did you mean? sso_secret
So it's relying on attr_accessor to implement the setter method.
This is obviously not an exhaustive test, so we should either add specs, or definitely do a manual test of the functionality. Hmm, but I can't find any documentation about this.. I wonder if it's even used...
There was a problem hiding this comment.
± To fix: the class methods (self.sso_secret etc) seem unnecessary, so I'd consider removing them instead. And why not move :sso_secret, :sso_url to the ACCESSORS list..
But still I'd like to confirm if it's worth it first.
There was a problem hiding this comment.
Update: this whole file is unused and so I've scheduled for removal.
For now, I suggest we add rubocop:disable for this whole file and move on.
| attr_accessor(*ACCESSORS, :sso_secret, :sso_url) | ||
| attr_accessor(*ACCESSORS) | ||
|
|
||
| def self.sso_secret | ||
| raise "sso_secret not implemented on class, be sure to set it on instance" |
There was a problem hiding this comment.
Update: this whole file is unused and so I've scheduled for removal.
For now, I suggest we add rubocop:disable for this whole file and move on.
|
Hey @anansilva 👋 Do you want to continue this? |
What? Why?
Fixed the
Lint/DuplicateMethodsoffense.sso_secretis defined at bothattr_accessorandlib/discourse/single_sign_on.rb:66sso_urlis defined at bothattr_accessorandlib/discourse/single_sign_on.rb:70What should we test?
Fix seems pretty safe since we are just removing duplicate instance methods. But might be worth testing SSO to discourse.
Release notes
Changelog Category: Technical changes
Dependencies
Documentation updates