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

SP-107,108,109 Bulk promo codes #285

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

Conversation

tomdonarski
Copy link
Contributor

@tomdonarski tomdonarski commented Nov 13, 2023

What?

This PR extends the bulk promo codes feature with administrative functionality through the admin panel.

Handling and administrating promotion batches:
Create batch:

create.batch.mov

Populate batch:

populate.batch.mov

Promotion codes export to CSV:

promo.codes.export.to.csv.mov

Promotion codes import from CSV:

promo.codes.import.from.csv.mov

Populate batch with options:

populate.batch.with.options.mov

Why?

New feature

@tomdonarski tomdonarski marked this pull request as ready for review November 17, 2023 06:06
Gemfile Outdated
@@ -64,4 +64,5 @@ end
spree_opts = { github: 'spree/spree', branch: 'main' }
gem 'spree_core', spree_opts
gem 'spree_api', spree_opts
gem 'sidekiq'
Copy link
Member

Choose a reason for hiding this comment

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

Please don't add sidekiq as a dependency - Spree::Core defines a Spree::ApplicationJob as a base for async jobs. It's based on ApplicationJob, which means that if an application already uses a different job queue, they will be able to keep using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Thanks!

@@ -0,0 +1,7 @@
class CreatePromotionBatch
Copy link
Member

Choose a reason for hiding this comment

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

As I've mentioned before, this should live in Spree::Core, not in Spree::Backend - as it's not strictly related to the admin panel. Otherwise we won't be able to reuse this logic e.g. in the API.

@tomdonarski tomdonarski force-pushed the feature/promotion-cloning branch from 87342e8 to 6b57a51 Compare December 4, 2023 07:42
@tomdonarski tomdonarski changed the title SP-109 Clone promotions asynchronously SP-107,108,109 Bulk promo codes Dec 12, 2023
@@ -40,6 +40,7 @@ def collection
params[:q][:s] ||= 'id desc'

@collection = super
@collection = @collection.where(promotion_batch_id: nil)
Copy link
Member

Choose a reason for hiding this comment

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

Why's that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order not to display the batched promotions. This would effectively flood the index view, and render it unusable.

config/routes.rb Outdated
@@ -10,6 +10,14 @@

resources :promotion_categories, except: [:show]

resources :promotion_batches do
Copy link
Member

Choose a reason for hiding this comment

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

Do we support e.g. removing an existing promotion batch? If yes, what it the consequence of this action for the promotions that were created as a part of this batch?

Copy link
Contributor Author

@tomdonarski tomdonarski Dec 22, 2023

Choose a reason for hiding this comment

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

Empty batches (with and without a template promotion assigned) can be deleted. For non-empty batches an error is displayed, and the cascading deletion is rolled back.

Copy link
Contributor Author

@tomdonarski tomdonarski Dec 22, 2023

Choose a reason for hiding this comment

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

What would the desired behaviour be? Delete/soft-delete the associated promotions? Or should it stay like this until the associated promotions are deleted?
Note: the latter would require adding an option to manage a bulk delete (currently, an update to the template propagates to the batched promotions by recreating them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As currently a promotion can be deleted regardless of the fact whether it's been used or not (also, when it's not depleted its limit yet), so I've added a service for deleting a batch.

redirect_back fallback_location: admin_promotions_path, alert: e.message
end

def duplicate
Copy link
Member

Choose a reason for hiding this comment

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

Is that duplicate or populate (following the convention you've used for the service objects)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spot on! Changed it 🙂

@tomdonarski tomdonarski force-pushed the feature/promotion-cloning branch from 42bf39b to 94743b0 Compare January 7, 2024 10:37
@rafalcymerys rafalcymerys force-pushed the feature/promotion-cloning branch 2 times, most recently from 7c0f51e to 2984a67 Compare February 14, 2024 15:20
@rafalcymerys rafalcymerys force-pushed the feature/promotion-cloning branch from 2984a67 to 70e43f0 Compare February 14, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants