AO3-6467 Log on kin side when a user with a fnok is deleted #4633
Conversation
Instead of trying to infer them From mirroring the "active" actions
github-actions
bot
added
Has Migrations
app/models/user.rb
Outdated
| @@ -180,6 +180,11 @@ def log_removal_as_next_of_kin | |||
| fnok_user_id: self.id | |||
| }) | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
Would it be neater if the logging methods log_removal_as_next_of_kin and log_next_of_kin_removed went into users_helper?
We could name them as
log_removal_as_next_of_kin and
log_admin_removal_as_next_of_kin to be clear about the distinction.
I also think having some brief comments/named variables would be helpful for this method, as the ...kin.kin might be confusing.
There was a problem hiding this comment.
I think the content of the app/helpers folder, including users_helper is only supposed to be for methods used in views?
Regardless, moving all that mess to its own file indeed seems like a goo idea.
Though not a view helper
brianjaustin
added
the
Priority: High - Broken on Test
lib/user_history.rb
Outdated
| @@ -0,0 +1,52 @@ | |||
| module UserHistory | |||
| def log_assignment_of_next_of_kin(user, kin, admin:) | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I think the : is to mark admin as a (required) keyword argument, rather than a positional one: https://docs.ruby-lang.org/en/master/syntax/methods_rdoc.html#label-Keyword+Arguments
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-6467?focusedCommentId=360484
Purpose
The logging of fannish next of kin addition and removal as previously coded was trying to be too clever, only saving data on the forebear side then inferring what to display on their successor side from that. This caused not only convoluted code but also bugs in the context of user deletion.
This PR moved to a more straightforward if more verbose approach by explicitly logging any change on both sides.
Testing Instructions
As per the JIRA
References
Untouched (as the visible behavior didn't change save for one bugfix) but nonetheless relevant end-to-end tests
Credit
Ceithir (he/him)
Note: weeklies (she/her) also contributed code to this feature in a previous PR