Promo code batch join chars #2662
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
houndci-bot
reviewed
Mar 29, 2018
| @@ -0,0 +1,9 @@ | |||
| class AddJoinCharactersToPromotionCodeBatch < ActiveRecord::Migration[5.1] | |||
There was a problem hiding this comment.
Missing magic comment # frozen_string_literal: true.
gevann
force-pushed
the
promo-code-batch-join-chars
branch
from
2c56732 to
393adcf
Compare
houndci-bot
reviewed
Mar 29, 2018
| @@ -0,0 +1,10 @@ | |||
| # frozen_string_literal: true | |||
| class AddJoinCharactersToPromotionCodeBatch < ActiveRecord::Migration[5.1] | |||
There was a problem hiding this comment.
Add an empty line after magic comments.
houndci-bot
reviewed
Mar 29, 2018
| @@ -0,0 +1,10 @@ | |||
| # frozen_string_literal: true | |||
| class AddJoinCharactersToPromotionCodeBatch < ActiveRecord::Migration[5.1] | |||
There was a problem hiding this comment.
Add an empty line after magic comments.
gevann
force-pushed
the
promo-code-batch-join-chars
branch
from
393adcf to
c673353
Compare
kennyadsl
requested changes
Mar 30, 2018
| @@ -6,7 +6,8 @@ class PromotionCodeBatchJob < ActiveJob::Base | |||
|
|
|||
| def perform(promotion_code_batch) | |||
| PromotionCode::BatchBuilder.new( | |||
| promotion_code_batch | |||
| promotion_code_batch, | |||
| join_characters: promotion_code_batch.join_characters | |||
There was a problem hiding this comment.
since the join_characters is included into the promotion_code_batch we are initializing the BatchBuilder with, what's the purpose of also passing it as a keyword argument? I think it could be better to keep the .new identical and work inside the BatchBuilder class to make the join_characters an instance attribute instead of a class level one
added 3 commits
April 3, 2018 10:01
This commit adds a new string field to the PromotionCodeBatch model,
join_characters, in order to allow customizing which characters should
be used in promotion batch creation.
Default has been set to an underscore ('_') to keep in line with current
behavior.
Now that Spree::PromotionCodeBatch has a join_characters field, the job which creates the batch of promotions should use this field.
gevann
force-pushed
the
promo-code-batch-join-chars
branch
from
c673353 to
7e51e78
Compare
|
Suggested changes have been made, thanks! |
kennyadsl
approved these changes
Apr 4, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to resolve #1946, allowing customization of promotion separator characters (currently always
'_') via the admin. It stores the characters to use on thePromotionCodeBatchmodel, and has a default set to'_', so that users who do not wish to make use of this functionality do not need to change their workflows.