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

Fix promotion code batch #5383

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DanielePalombo
Copy link
Contributor

@DanielePalombo DanielePalombo commented Sep 14, 2023

Summary

When a new promotion with 2.5m new promo codes has created, it just generated about 250k promo codes and then stopped.

The PromotionBatch screen this error: Errored: #<ActiveRecord::RecordInvalid: Validation failed: Value has already been taken>

This PR rescues the exception raised by the Spree::PromotionCode creation and jumps to the next code to save, avoiding stopping the process completely.
The PR also adds the ability to restart the PromotionCodeBatch from the latest PromotionCodes created.

Fixes: #5379

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

Before this fix, when the Spree::PromotionCode creation failed, the
exception was raised and the job fails with error.
This commit rescues the creation exception and remove the failed code
from the codes list.
@DanielePalombo DanielePalombo requested a review from a team as a code owner September 14, 2023 14:36
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Sep 14, 2023
@DanielePalombo DanielePalombo changed the title Dp/improve promotion code batch Fix promotion code batch Sep 14, 2023
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we never reach the number of codes needed? The validation might fail consistently and we will end up in an infinite loop, no?

@@ -39,13 +39,15 @@ def generate_random_codes
new_codes = Array.new(max_codes_to_generate) { generate_random_code }.uniq
codes_for_current_batch = get_unique_codes(new_codes)

codes_for_current_batch.each do |value|
codes_for_current_batch = codes_for_current_batch.map do |value|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collecting results of large code counts can exceed available memory pretty quickly.

Comment on lines +43 to +51
codes_for_current_batch = codes_for_current_batch.map do |value|
Spree::PromotionCode.create!(
value: value,
promotion: promotion,
promotion_code_batch: promotion_code_batch
)
end
rescue ActiveRecord::RecordInvalid
nil
end.compact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address @tvdeyen's concern, what about something like:

codes_for_current_batch.filter! do |value|
  Spree::PromotionCode.create!(
    value: value,
    promotion: promotion,
    promotion_code_batch: promotion_code_batch
  )
rescue ActiveRecord::RecordInvalid
  false
end

This should maintain the existing performance characteristics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PromotionCodeBatch fails generating million codes
3 participants