Skip to content
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

Replace use of return keyword as logic control inside AR transactions, as this is not supported in Ruby 3 #52

Merged
merged 5 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions lib/draft_approve/models/draft_transaction.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,28 @@
#
# @return [Boolean] +true+ if the changes were successfully applied
def approve_changes!(reviewed_by: nil, review_reason: nil)
result = false
begin
ActiveRecord::Base.transaction do
self.lock! # Avoid multiple threads applying changes concurrently
return false unless self.status == PENDING_APPROVAL

drafts.order(:created_at, :id).each do |draft|
draft.apply_changes!
self.lock! # Avoid multiple threads applying changes concurrently
Dismissed Show dismissed Hide dismissed
duknic marked this conversation as resolved.
Show resolved Hide resolved
if self.status == PENDING_APPROVAL
Dismissed Show dismissed Hide dismissed
drafts.order(:created_at, :id).each do |draft|
Dismissed Show dismissed Hide dismissed
draft.apply_changes!
end

Dismissed Show dismissed Hide dismissed
self.update!(status: APPROVED, reviewed_by: reviewed_by, review_reason: review_reason)
Dismissed Show dismissed Hide dismissed
Dismissed Show dismissed Hide dismissed
result = true
next
end

self.update!(status: APPROVED, reviewed_by: reviewed_by, review_reason: review_reason)
return true
end
rescue StandardError => e
# Log the error in the database table and re-raise
self.update!(status: APPROVAL_ERROR, error: "#{e.inspect}\n#{e.backtrace.join("\n")}")
raise
end
result
end

Dismissed Show dismissed Hide dismissed

# Reject all changes in this +DraftTransaction+.
#
Expand Down
26 changes: 15 additions & 11 deletions lib/draft_approve/persistor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
if validate_model?(options) && model.invalid?
raise(ActiveRecord::RecordInvalid, model)
end

result = false
DraftApprove::Transaction.ensure_in_draft_transaction do
# Now we're in a Transaction, ensure we don't get multiple drafts for the same object
if model.persisted? && Draft.pending_approval.where(draftable: model).count > 0
Expand Down Expand Up @@ -76,17 +76,21 @@
changes = serializer.changes_for_model(model)

# Don't write no-op updates!
return false if changes.empty? && action_type == Draft::UPDATE

return model.draft_pending_approval = Draft.create!(
draft_transaction: draft_transaction,
draftable_type: draftable_type,
draftable_id: draftable_id,
draft_action_type: action_type,
draft_changes: changes,
draft_options: draft_options
)
if changes.empty? && action_type == Draft::UPDATE
Dismissed Show dismissed Hide dismissed
next
else
Dismissed Show dismissed Hide dismissed
model.draft_pending_approval = Draft.create!(
draft_transaction: draft_transaction,
Dismissed Show dismissed Hide dismissed
draftable_type: draftable_type,
draftable_id: draftable_id,
draft_action_type: action_type,
draft_changes: changes,
draft_options: draft_options
)
result = model.draft_pending_approval
end
end
result
end

# Write the changes represented by the given +Draft+ object to the database.
Expand Down
Loading