For a date check, I would expect the start date to map to a timestamp of 12:00 AM on the start date and the end date to map to 12:00 AM on the day following the end date. (or, if you will, 0:00 of day 1 to 0:00 of day 2) I had a look at the condition this module is using for date checks, and it simply does a >= against date1 and a <= against date2. If I said a discount should start on October 20th and end on October 21st, as I understand the logic, it would actually only be good for October 20th. If I just wanted the 20th, though, it should start and end on October 20th.

This wouldn't be a problem if discount dates were more granular - in other words, if the selector included hour along with date. As it stands now, it's opaque what data is actually being stored, and it's opaque how exactly the condition works in code.

See: #2115297: Why are we using so many custom conditions and actions?

CommentFileSizeAuthor
#9 2115295-9.discount_end_date.patch15.85 KBrszrama
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Issue summary: View changes

Linking to a related issue.

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang commented

Yes. We need to support "time" and more importantly we need solve #2115297: Why are we using so many custom conditions and actions?

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold commented

Here's a use case: Cyber Monday.

For now I'm just going to write custom rules (or modify the ones created by this module) but I like the ideas in the linked issue — I would like for this module to be a user-friendly interface to Rules so that content administrators can easily create discounts without having to touch or understand Rules.

rszrama’s picture

rszrama CreditAttribution: rszrama commented

Yeah, I'm prepping a Black Friday sale myself. Gotta move that cheddar (literally... as I sell cheese ; ).

kevster’s picture

kevster CreditAttribution: kevster commented

I just spotted this issue - I set a discount rule up for a week over Xmas - it kicked in ok on the 24th and I set it to finish on the 31st - expecting it to finish at midnight. It finished at 12 midday which I dont understand the logic behind?

I checked the server time/date and thats fine. Question is why would it finish half way through the day if it only works off dates and not time?

kevster’s picture

kevster CreditAttribution: kevster commented

Any news on this please - I dont understand why the finish time would be half way through the day e.g. 12:00 midday? Surely should start at midnight e.g. 00:01 and finish at midnight e.g. 23:59? If so you could put start date and end date the same and it would cover one day (24 hours).

Time in addition to date would be an extra bonus but for now I dont see the logic behind how the dates work?

mihai_brb’s picture

mihai_brb CreditAttribution: mihai_brb commented
Issue summary: View changes

What about a settings form for for the commerce_discount_date field? Simply calling the field_ui_field_settings_form would be enough.
Since we do not have manage fields for commerce discount, maybe under a path like 'admin/commerce/store/discounts/discount-date'?

Mihai

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: +sprint, +Needs tests
fonant’s picture

fonant CreditAttribution: fonant at Fonant Ltd commented

I understood the code to be comparing days, with "or equal to" so including the start and end days specified. But enabling times to be specified might be useful.

EDIT: I was wrong, it's comparing timestamps of 0:00:00 on each day. So the first date is included in the period, but the second date is not.

What about time zones...?

rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented
Status: Active » Needs review
FileSize
2115295-9.discount_end_date.patch15.85 KB

Patch attached updates the form element to add descriptions re: to the times discounts start / end on the selected days. It also fixes a display issue with the start date title and moves us toward cleaning up our .scss. Any checks against the end date now add 86400 seconds, which is a timezone safe way of checking for anything up to a day after the date field value. Oh, and instead of time(), we're using REQUEST_TIME per best practices.

rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented
Status: Needs review » Fixed

Tests pass, though admittedly I doubt we have one spoofing start / end dates. We should fix that once we merge Discount Date into Discount. I don't think it's worth adding tests to the individual submodules until that happens, so I'll open another issue for that task.

Committing and following up with more Discount Date fixes in other issues.

  • rszrama committed 42b5c6a on 7.x-1.x 
    Issue #2115295 by rszrama: Date checks are too simplistic - cannot be...
torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented

You are on fire lately Ryan. This is an awesome addition, and we'll definitely be testing it out for Black Friday / Cyber Monday this year! Thanks!

Perignon’s picture

Perignon CreditAttribution: Perignon commented

This is badass! I will be giving it a run for Cyber Monday too!

BrightBold’s picture

BrightBold
she/her
Primary language English
Location Boston, MA
CreditAttribution: BrightBold as a volunteer commented

Yes! Just in time for Cyber Monday! Thanks Ryan. (And now I'm hankerin' for a hunka cheese, per #3.)

rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented

Hah! Sadly we shut 'er down this year. Our primary farmer stopped making his cheese and the remaining farmer wasn't interested in taking over the site. : P

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

haggins’s picture

haggins CreditAttribution: haggins at Berger Schmidt commented

This patch doesn't seem to be included in current 7.x-1.x, is it? At least, I'm still not able to apply single day discounts.

The patch modifies modules/commerce_discount_date/commerce_discount_date.module which does not exist in the repository.

Any hints?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@haggins, in the 1.x branch that module was merged into the main module.

haggins’s picture

haggins CreditAttribution: haggins at Berger Schmidt commented

Thank you! I've opened a new issue for this: #2738671: One day discounts not possible