-
Notifications
You must be signed in to change notification settings - Fork 985
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
Fixes #16363 - fix taxable user deletion #4879
Conversation
Issues: #16363 |
app/models/usergroup_member.rb
Outdated
raise "Unknown member type #{member_type}" | ||
end | ||
end | ||
alias_method_chain :member, :unscoped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alias_method_chain
will be gone in Rails 5.1, see 3896497
ef9f33c
to
e22b5ae
Compare
👍 for both the code and functionality. I tested the patch today and it works nicely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ares: Looks good, just one small comment.
app/models/usergroup_member.rb
Outdated
when 'Usergroup' | ||
Usergroup.unscoped { super } | ||
else | ||
raise "Unknown member type #{member_type}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... Foreman::Exception.new()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. We raise it as ArgumentError
on two other locations in this file. Whatever exception type we use, we should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I changed the exception to ArgumentError since this is an internal exception for devs, not really for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/FileName has the wrong namespace - should be Naming .rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming .rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming .rubocop.yml: Style/MethodName has the wrong namespace - should be Naming .rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming Error: The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`. (obsolete configuration found in .rubocop.yml, please update it)
@ares: Can you rebase please to fix the test failures? Thanks. |
rebased |
test failures related ;) |
ah I accidentally pushed older version from a different computer, should be good now |
@ares: Before I merge this, one final question, just to make sure: Is there a case when |
no, I'm not aware such case, it's always user or user group |
Merged, thanks @ares and @tstrachota. I'll put this on 1.17, but feel free to put it in 1.16 at your own discretion. |
Thanks, I think it's fine to keep for 1.17, it's not 1.16 regression. |
Reproducing steps:
as admin:
with this fix, the user gets deleted
technical details:
belongs_to respects the default scope which unfortunately is used by taxonomies, therefore when member is being loaded, it fails resulting in member being nil. When it's later passed to
find_all_affected_users_for
, it fails sincenil
is unknown member type. In this case we need to skip taxonomies verification. Sadly we can't use any of common tricks since this is a polymorphic associations.belongs_to :member, -> { unscope(:where) }, :polymorphic => true
does not work since it also reset the where defined by polymorphic part, taxonomix does not define any named scope, justwhere("id IN ...")