-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add a configurable order number generator #1820
Conversation
core/spec/models/spree/order_spec.rb
Outdated
it { is_expected.to eq String } | ||
context "with order number generator configured" do | ||
class TruthNumberGenerator | ||
def initialize(options = {}) |
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.
Put empty method definitions on a single line.
# Use the random number if no other order exists with it. | ||
if Spree::Order.exists?(number: random) | ||
# If over half of all possible options are taken add another digit. | ||
length += 1 if Spree::Order.count > (10**length / 2) |
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.
Useless assignment to variable - length.
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 is a good catch by hound. I think there's an issue here between length
being a instance method and being assigned here as 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.
Oh, actually a good catch. So, the only solution I see is to use a attr_accessor
for length
then, right?
I like this. It would be even better if we could share the code to generate Order numbers, Shipment numbers, and Payment numbers |
@jhawthorn I also thought about this, but I think we should not make the assumption that all numbers generate the same way. The only idea coming to my mind is to ask the |
085bc83
to
680860c
Compare
@jhawthorn addressed the issues. Still think this is valuable as order number generator on its own. Shipments and friends get their number through Let's address this - totally valid concern - in a separate PR, ok? |
# @api experimental | ||
attr_writer :order_number_generator | ||
def order_number_generator | ||
@order_number_generator ||= Spree::Order::NumberGenerator |
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.
One thought: maybe this should be an instance of NumberGenerator
rather than the class. This would allow developers to specify a length, prefix, or letters without needing to write their own 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.
LGTM, but have a suggested improvement
The default order number generator was extracted into a class. Provide your own class instance that takes an options hash and returns a string when called for `#generate`. Spree::Config.order_number_generator = TrueNumberGenerator.new class TrueNumberGenerator def initialize(options = {}) end def generate '42' end end
680860c
to
92622c5
Compare
@jhawthorn good idea. I needed to remove the possibility for passing |
The default order number generator was extracted into a class.
Provide your own class that takes an options hash and returns a
string when called for
#generate
.