Follow-up from "Fix Bitbucket Cloud importer pull request state"
!19734 (merged) fixed the problem with imported pull requests from Bitbucket would always be in the opened
state instead of closed
. This caused slowdowns on updating merge requests because GitLab scans for all open merge requests.
However, !19734 (merged) solved the problem by adding an override for initialize
in the Issuable
concern. This isn't ideal because state_id
is assigned twice: once in the initializer, and once in the state=
method. We should probably fix this issue in the state_machines-activerecord
gem via #35746 (comment 241148787):
diff --git a/lib/state_machines/integrations/active_record.rb b/lib/state_machines/integrations/active_record.rb
index 90366f5..412dea1 100644
--- a/lib/state_machines/integrations/active_record.rb
+++ b/lib/state_machines/integrations/active_record.rb
@@ -456,7 +456,7 @@ module StateMachines
define_helper :instance, <<-end_eval, __FILE__, __LINE__ + 1
def initialize(attributes = nil, *)
super(attributes) do |*args|
- scoped_attributes = (attributes || {}).merge(self.class.scope_attributes)
+ scoped_attributes = (self.attributes || {}).merge(self.class.scope_attributes)
self.class.state_machines.initialize_states(self, {}, scoped_attributes)
yield(*args) if block_given?
The following discussions from !19734 (merged) should be addressed:
-
@ayufan started a discussion: (+2 comments) This really leads that we should create a spec validation of DB schema that ensures that we never create a columns that are
name
+name_id
within a single table.As AR consider the
_id
specially. -
@ayufan started a discussion: (+7 comments) Would it be better to change
state_machine
?state_machine :state_id, initial: :opened do
and remove
initial:
? -
@engwan started a discussion: Since we're deleting this from
attr
, we would be passing bothstate
andstate_id
tosuper
. I think this is fine though. We'd just be unnecessarily triggeringstate=
but that shouldn't be an issue. -
@toon started a discussion: (+2 comments) While the fix isn't really pretty, it would fix the issue reliably. We can address the issue properly later, as @engwan suggested.