Remove any whitespace from a promo code. #1796

eric1234 · 2017-03-24T17:22:01Z

There is no legit reason to have spaces in codes as the user would have to duplicate the spacing when they entered the code during checkout.

I have gotten real world cases where a store admin has inadvertently included a space before/after the code (due to a copy/paste error), as well as space within the code (due to font size making it hard to
see the space).

I considered putting a validation in place to correct the user rather than silently modifying their code but we are already silently modifying by downcasing the code. Even after being told of a spacing problems,
some users might have difficultly determing where the space is or understanding the error message. In the interest of just doing what they likely mean, I think silently modifying is the lesser of two evils.

houndci-bot · 2017-03-24T17:22:10Z

core/app/models/spree/promotion_code.rb

@@ -39,4 +40,8 @@ def usage_limit
  def downcase_value
    self.value = value.downcase
  end
+
+  def remove_excess_whitespace
+    value&.gsub! /\s+/, ''


unexpected token error
ambiguous first argument; put parentheses or a space even after the operator

I assume the "ambiguous first argument" is indicating I should add parens. So gsub!(/\s+/, '') (under the idea that somehow that is clearer).

Is the unexpected token error referring to the safe navigation operator? Is this a project decision (compat with older Ruby) or just hound not knowing the more modern syntax?

It's compat with older Ruby. Solidus currently only requires Ruby 2.2.2 so we can't use the safe navigation operator.
https://github.com/solidusio/solidus/blob/1a556fb/solidus.gemspec#L15

We already have a validation for code presence so I don't think we need to consider nil values. Also I think we should use self.value = value.strip (not bang method and not worry about internal whitespace). Open to others' thoughts on that.

Up to you guys regarding the internal whitespace. I will say I have had a real world case where the admin put whitespace in the middle and couldn't see it because of the small font size. The question is how far do we go to make it fool proof since there is always a bigger fool?

We do know whitespace should never be in a code so doesn't seem to hurt to remove it. The person who pays my paycheck is the one who added the whitespace so if you prefer strip I'll just have to do a local override.

I think .strip can be enough for fixing the main issue (whitespace before or after). Each store will be able to override the method easily if needed.

houndci-bot · 2017-03-24T17:22:10Z

core/app/models/spree/promotion_code.rb

@@ -39,4 +40,8 @@ def usage_limit
  def downcase_value
    self.value = value.downcase
  end
+
+  def remove_excess_whitespace
+    value&.gsub! /\s+/, ''


unexpected token error
ambiguous first argument; put parentheses or a space even after the operator

jordan-brough

This seems like a good addition, but I left a couple thoughts.

jordan-brough · 2017-03-30T18:52:48Z

core/app/models/spree/promotion_code.rb

@@ -39,4 +40,8 @@ def usage_limit
  def downcase_value
    self.value = value.downcase
  end
+
+  def remove_excess_whitespace
+    value&.gsub! /\s+/, ''


We already have a validation for code presence so I don't think we need to consider nil values. Also I think we should use self.value = value.strip (not bang method and not worry about internal whitespace). Open to others' thoughts on that.

kennyadsl

@eric1234 sorry for the late response, I think we can carry this on if you have time to change code following the strip suggestion.

kennyadsl · 2017-10-04T12:02:06Z

core/app/models/spree/promotion_code.rb

@@ -39,4 +40,8 @@ def usage_limit
  def downcase_value
    self.value = value.downcase
  end
+
+  def remove_excess_whitespace
+    value&.gsub! /\s+/, ''


I think .strip can be enough for fixing the main issue (whitespace before or after). Each store will be able to override the method easily if needed.

mamhoff

I think this is fine. strip would work as well, and is potentially less intrusive. However, this works as well as strip, and I'm worried we're bike shedding here. I think this can go in as-is. @jordan-brough @kennyadsl ?

kennyadsl · 2017-10-04T12:48:16Z

Talking with @mamhoff he convinced me that the current version is good enough to be merged. Having promo codes with spaces inside is not a documented feature so we should not care that much about it.

jhawthorn

I don't think this should be done.

Users shouldn't enter a value and have it changed without them noticing. An admin might really want to enter "SAVE MORE" as their coupon code. If we don't want to allow it, that's fine, but it should be done as a validation (or using html5 form inputs/JS) rather than surprising users.

I'm also concerned that this could modify existing values in the database if for any reason a promotion_code was saved.

TL;DR I'd prefer a validation or a JS/html change instead.

eric1234 · 2017-10-05T21:50:19Z

The last paragraphs in my original PR message explained why I opted for silently modifying over explicit validation. I saw it as the lesser of two evils but that is a bit of opinion. If you guys prefer validation then go ahead and close the PR.

tvdeyen

I think strip is ok, but I don't see any reason why we should not allow whitespace inside the code.

houndci-bot · 2017-10-09T01:13:04Z

core/spec/models/spree/promotion_code_spec.rb

+      end
+
+      context 'with extra spacing' do
+        let(:code) { ' new code '}


Space missing inside }.

houndci-bot · 2017-10-09T01:13:04Z

core/spec/models/spree/promotion_code_spec.rb

+      end
+
+      context 'with extra spacing' do
+        let(:code) { ' new code '}


Space missing inside }.

eric1234 · 2017-10-09T01:18:24Z

I didn't expect so a small little change to make so much discussion. To get this thing closed and move onto more exciting things I have updated the PR to strip so that spaces within a code are supported.

I also went ahead and combined it with the existing downcasing method to reduce the clutter.

As if you should merge this or use @jhawthorn validation version (or both) I'll leave that up to you guys. I prefer the system just do what they mean rather than having the user try to remove characters they cannot see but understand if you prefer it the other way.

Regardless the PR is updated so it is ready if you do want to merge. If not then feel free to close.

mamhoff · 2017-10-09T08:49:11Z

Hey @eric1234 thank you, very nice work (and yeah, this has generated a lot of discussion).

Would you mind merging the three commits into one (and removing the code nazi language)?

Thank you again for your patience and hard work.

There is no legit reason to have spaces in codes as the user would have to duplicate the spacing when they entered the code during checkout. I have gotten real world cases where a store admin has inadvertently included a space before/after the code (due to a copy/paste error). I have also gotten cases where the admin has accidentally put spaces within a code and could not tell because of the small font. After discussion it was decided we will not remove these due to the possibility that some stores may want spaces in their code. I considered putting a validation in place to correct the user rather than silently modifying their code but we are already silently modifying by downcasing the code. Even after being told of a spacing problems, some users might have difficultly determining where the space is or understanding the error message. In the interest of just doing what they likely mean, I think silently modifying is the lesser of two evils.

eric1234 · 2017-10-09T11:03:57Z

K, squashed and rebased to master. My frustration with overzealous code linters removed. :)

tvdeyen

Fine now

houndci-bot reviewed Mar 24, 2017

View reviewed changes

jordan-brough suggested changes Mar 30, 2017

View reviewed changes

kennyadsl requested changes Oct 4, 2017

View reviewed changes

mamhoff approved these changes Oct 4, 2017

View reviewed changes

kennyadsl approved these changes Oct 4, 2017

View reviewed changes

jhawthorn suggested changes Oct 5, 2017

View reviewed changes

jhawthorn mentioned this pull request Oct 5, 2017

Use HTML5 to prevent spaces in promotion codes #2271

Closed

tvdeyen requested changes Oct 6, 2017

View reviewed changes

houndci-bot reviewed Oct 9, 2017

View reviewed changes

eric1234 force-pushed the clean_whitespace_on_promo_codes branch from a847aff to 7c01d19 Compare October 9, 2017 11:02

tvdeyen approved these changes Oct 11, 2017

View reviewed changes

jhawthorn approved these changes Oct 25, 2017

View reviewed changes

jordan-brough approved these changes Oct 25, 2017

View reviewed changes

jordan-brough merged commit 52653ae into solidusio:master Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove any whitespace from a promo code. #1796

Remove any whitespace from a promo code. #1796

eric1234 commented Mar 24, 2017

houndci-bot Mar 24, 2017

eric1234 Mar 24, 2017

jordan-brough Mar 30, 2017 •

edited

Loading

jordan-brough Mar 30, 2017

eric1234 Mar 31, 2017

kennyadsl Oct 4, 2017

houndci-bot Mar 24, 2017

jordan-brough left a comment

jordan-brough Mar 30, 2017

kennyadsl left a comment

kennyadsl Oct 4, 2017

mamhoff left a comment

kennyadsl commented Oct 4, 2017

jhawthorn left a comment

eric1234 commented Oct 5, 2017

tvdeyen left a comment

houndci-bot Oct 9, 2017

houndci-bot Oct 9, 2017

eric1234 commented Oct 9, 2017

mamhoff commented Oct 9, 2017

eric1234 commented Oct 9, 2017

tvdeyen left a comment

Remove any whitespace from a promo code. #1796

Remove any whitespace from a promo code. #1796

Conversation

eric1234 commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordan-brough Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordan-brough left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

kennyadsl commented Oct 4, 2017

jhawthorn left a comment

Choose a reason for hiding this comment

eric1234 commented Oct 5, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric1234 commented Oct 9, 2017

mamhoff commented Oct 9, 2017

eric1234 commented Oct 9, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

jordan-brough Mar 30, 2017 •

edited

Loading