Investigate removing `ApproverMigrateHook`
ApproverMigrateHook
has a comment to the effect that it can be removed "once #1979 (closed) is closed". #1979 (closed) has been closed for a couple years at this point, as confirmed by @lulalala
It should be removed when we are sure codes that create
Approver
andApproverGroup
directly are no longer used (e.g. API endpoints). And I hope during the two years we haven't add anything toApprover
andApproverGroup
that can go against this.
I stumbled into this while working on ApprovalRules
(!48511 (comment 465385125)) and was surprised by the way this concern exists only to call out to Gitlab::BackgroundMigration::MigrateApproverToApprovalRules
, which creates divergent shadow or ghost classes in the middle of an active process that use the same DB tables as existing AR models from the main codebase, and performs DB writes using these new classes, which caused callbacks and validations to be ignored (since they no longer exist, belonging to the original version of the class.)
We should investigate whether we can remove this all-together, or if not, if extracting the mechanics of the BackgroundMigration
into a service object (or just in ApproverMigrateHook
itself) so that at the very least we're still applying callbacks and validations as the main AR classes expect.. and remove (or reduce) a potentially mysterious "records created as side effect" bit of code.