-
-
Notifications
You must be signed in to change notification settings - Fork 592
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
fix History + SequentiallySlugged + Scope issues #877
Conversation
base_class = Slug | ||
else | ||
base_class = self.class.base_class | ||
end |
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.
could you extract this into a method base_class
?
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.
@parndt done
@@ -7,11 +7,17 @@ def self.setup(model_class) | |||
def resolve_friendly_id_conflict(candidate_slugs) | |||
candidate = candidate_slugs.first | |||
return if candidate.nil? | |||
SequentialSlugCalculator.new(scope_for_slug_generator, | |||
scope = scope_for_slug_generator |
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.
this could probably go back to being used inline?
SequentialSlugCalculator.new(scope_for_slug_generator,
because it doesn't get re-used so it doesn't appear to need to be a variable
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.
@parndt done
This have been running for 3 weeks in production, the only problem I've found is that I have |
Thanks a lot @kshnurov 👍 |
SequentiallySlugged
withHistory
doesn't look into previously used slugs, which can result in a wronglast_sequence_number
and failure.