Add delegations

[tanmay3011]

No description provided.

[cbrunsdon]

👍, thanks @tanmay3011! Really appreciate the effort trying to clean up core.

[Senjai]

Did we want this allow_blank: true to be a part of the uniqueness clause?
validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at], allow_blank: true }

Same question for other calls below.

[Senjai]

Ah, this is valid. allow_blank can be passed directly into the UniquenessValidator or used as an option to the validates method. The difference being whether or not the validators are invoked, or are expected to handle :allow_blank directly (which uniqueness does). I've only ever passed allow_blank into specific validators, I didn't know you could short circuit using it outside of those validation statements. 👍

[Senjai]

Good work! @tanmay3011

[athal7]

👍. Was a bit thrown by the allow_blank and allow_nil paired with the presence validation. The performance benefits make sense to me, I wonder if a comment would help to not trip up future contributors though?

[tanmay3011]

@athal7 Where should I add comment to each file where I have used allow_blank or to any specific place

[athal7]

@tanmay3011 hm yea that does seem excessive. would it save the extra query if you removed the presence validation instead and changed allow_blank to false? that would be a bit less confusing IMO.

If not, 👍

[tanmay3011]

@athal7 presence validation shouldn't be removed since case of nil won't be handled in uniqueness validation. So presence validation is a must.

[athal7]

@tanmay3011 I think I'm missing where the query count is reduced, can you say more? Here's my own little test:

before adding allow_blank:

irb(main):001:0> Spree::AdjustmentReason.create!(name: "foo", code: "bar")
   (0.0ms)  begin transaction
  Spree::AdjustmentReason Exists (0.1ms)  SELECT  1 AS one FROM "spree_adjustment_reasons" WHERE LOWER("spree_adjustment_reasons"."name") = LOWER('foo') LIMIT 1
  Spree::AdjustmentReason Exists (0.1ms)  SELECT  1 AS one FROM "spree_adjustment_reasons" WHERE LOWER("spree_adjustment_reasons"."code") = LOWER('bar') LIMIT 1
  SQL (0.3ms)  INSERT INTO "spree_adjustment_reasons" ("name", "code", "created_at", "updated_at") VALUES (?, ?, ?, ?)  [["name", "foo"], ["code", "bar"], ["created_at", "2015-12-07 14:26:44.287782"], ["updated_at", "2015-12-07 14:26:44.287782"]]
   (0.8ms)  commit transaction

after adding allow_blank:

irb(main):002:0> Spree::AdjustmentReason.create!(name: "foo", code: "bar")
   (0.1ms)  begin transaction
  Spree::AdjustmentReason Exists (0.1ms)  SELECT  1 AS one FROM "spree_adjustment_reasons" WHERE LOWER("spree_adjustment_reasons"."name") = LOWER('foo') LIMIT 1
  Spree::AdjustmentReason Exists (0.1ms)  SELECT  1 AS one FROM "spree_adjustment_reasons" WHERE LOWER("spree_adjustment_reasons"."code") = LOWER('bar') LIMIT 1
  SQL (0.4ms)  INSERT INTO "spree_adjustment_reasons" ("name", "code", "created_at", "updated_at") VALUES (?, ?, ?, ?)  [["name", "foo"], ["code", "bar"], ["created_at", "2015-12-07 14:20:24.484747"], ["updated_at", "2015-12-07 14:20:24.484747"]]
   (1.5ms)  commit transaction

You probably have a deeper understanding of this and I'm just missing where to look, but just want to see it through because of the introduced code complexity :).

[tanmay3011]

@athal7 Did you reload your console?
As far as to bring validation in affect you need to leave the name or code attribute blank

Without allow_blank

Spree::AdjustmentReason.create!(name: "", code: "")
   (0.1ms)  begin transaction
  Spree::AdjustmentReason Exists (0.1ms)  SELECT  1 AS one FROM "spree_adjustment_reasons" WHERE LOWER("spree_adjustment_reasons"."name") = LOWER('') LIMIT 1
  Spree::AdjustmentReason Exists (0.1ms)  SELECT  1 AS one FROM "spree_adjustment_reasons" WHERE LOWER("spree_adjustment_reasons"."code") = LOWER('') LIMIT 1
   (0.0ms)  rollback transaction
ActiveRecord::RecordInvalid: Validation failed: Name can't be blank, Code can't be blank

As you can see above when I left name and code field blank uniqueness validation still gets fired

With allow_blank

Spree::AdjustmentReason.create!(name: "", code: "")
   (0.1ms)  begin transaction
   (0.1ms)  rollback transaction
ActiveRecord::RecordInvalid: Validation failed: Name can't be blank, Code can't be blank

Uniqueness validation doesn't get fired. Only presence validation is enough to check in case record is left blank.

[athal7]

I did, and also entirely rebuilt the sandbox just to make double sure.
On Mon, Dec 7, 2015 at 1:22 PM Tanmay Sinha [email protected]
wrote:

@athal7 https://github.com/athal7 Did you reload your console?
Inorder to reload all your models again with the validation in affect

—
Reply to this email directly or view it on GitHub
#559 (comment).

[tanmay3011]

@athal7 Just see the example I posted. You need to leave name and code attribute blank in both cases to see the difference

[athal7]

@tanmay3011 thanks for the example, that helps / makes sense.

Since we are trying to prevent blank names and codes, my personal preference would be to introduce a client-side validation and avoid db interaction entirely rather than introducing this level of indirection. I know that doesn't cover us in the API, but personally I prefer that trade-off.

Curious to hear opinions of other core team members...

[cbrunsdon]

@athal7 I've got nothing against it. Its definitely a quirk/optimization of rails I hadn't considered before but am for it. @gmacdougall @jordan-brough @jhawthorn ?

[gmacdougall]

I'm not particularly bothered about the extra validation queries either way. I do like having validations live as close to the DB as possible, as front end validations can easily be bypassed.

[athal7]

@gmacdougall just to be clear, I'm not suggesting we remove any sort of validating code, just remove seemingly contradictory validations that are in place to reduce queries in cases that we don't expect. the presence: true would stay.

[gmacdougall]

Thanks @athal7 I wasn't clear on that as I was having trouble following the flow of the conversation here.

[tanmay3011]

@athal7 Do I need to do any changes now?

[athal7]

I would be in favor of the changes I recommended earlier, but not going to
stand in front of this if the rest of the core team feels strongly
otherwise.
On Sun, Dec 13, 2015 at 3:27 AM Tanmay Sinha [email protected]
wrote:

@athal7 https://github.com/athal7 Do I need to do any changes now?

—
Reply to this email directly or view it on GitHub
#559 (comment).

[jhawthorn]

I am in favour of the changes here as they are now. Though the allow_blank might read as contradictory, it does behave exactly as one would expect, ie. not checking checking uniqueness on an already obviously invalid record.

[tanmay3011]

@jhawthorn Ok! Will update the PR and remove conflicts

[jhawthorn]

Merged as 2ad8570. Thanks!