Skip to content
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

Daily cron expression with timezone offset displays incorrect day #313

Open
1 task done
khOst opened this issue Jan 15, 2024 · 8 comments
Open
1 task done

Daily cron expression with timezone offset displays incorrect day #313

khOst opened this issue Jan 15, 2024 · 8 comments

Comments

@khOst
Copy link

khOst commented Jan 15, 2024

Cron Expression
0 20 28 * * with a tzOffset set to 5

Expected Output
"At 01:00 AM, on day 29 of the month"

Actual Output
"At 01:00 AM, on day 28 of the month"

Prerequisites

  • The cron expression being passed in is a valid expression. cRonstrue does not validate expressions and assumes the one you pass is already valid. See the FAQ for more details.
@bradymholt
Copy link
Owner

Isn't this correct? With tzOffset set to 5 and hour set to 20, this would cause the time to move to the next day. This was implemented / changed in #296.

@khOst
Copy link
Author

khOst commented Jan 16, 2024

Sorry, expected and actual should be switched in places, I've updated the description. The time is expected to move to next day, but cRonstrue returns the same day, despite the offset.

@khOst
Copy link
Author

khOst commented Jan 16, 2024

Also for a negative tzOffset instead of returning the same day, it should return the previous day. For example, day 27 is expected here https://runkit.com/65a61ad4cca25b00082679bc/65a61ae3fdc4a20008bebfa5.

@bradymholt
Copy link
Owner

@maxtwardowski - Do you think changes in #296 need to be tweaked for this case?

@ZaneGeiser
Copy link

Hi @bradymholt, I just started using this package (tyvm for building it!) this week and noticed this same issue.

I wrote some test that I think would need to pass in order for this issue to be resolved.

    it("5 23 1 * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: 2,
      }), "At 01:05 AM, on day 2 of the month")
    });

    it("5 23 31 * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: 2,
      }), "At 01:05 AM, on day 1 of the month")
    });

    it("5 23 1 * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: -2,
      }), "At 11:05 PM, on day 31 of the month")
    });

I went to try making the code change but noticed there are some special day-of-month cases (like L, WL, and LW), and wasn't sure how it would need to fit in with those cases.

@ZaneGeiser
Copy link

As I think about it more, it probably needs to wrap the L day of the month forward to 1 if the tzOffset pushes over the dateline.

Ie, if L is converted to the day before the last day, then it should remain as the Last day. But if L is converted to the day after the last day then it should write 'day 1 of the month'.

Or as tests:

    it("5 23 L * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: -2,
      }), "At 01:05 AM, on the last day of the month")
    });

    it("5 23 L * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: 2,
      }), "At 01:05 AM, on day 1 of the month")
    });

@kbazzani
Copy link

kbazzani commented Jul 2, 2024

First, very useful package, keep up the good work!

Second, here are a couple more examples. Along with the day of the month as previously mentioned, the month also doesn't wrap correctly.

construe.toString( "59 23 31 12 3", { locale:"en", tzOffset: 1} )
'At 12:59 AM, on day 31 of the month, and on Thursday, only in December'

Should be Jan 1st

construe.toString( "01 00 01 01 3", { locale:"en", tzOffset: -1} )
'At 11:01 PM, on day 01 of the month, and on Tuesday, only in January'

Should be Dec 31st.

Best!

@ZaneGeiser
Copy link

ZaneGeiser commented Jan 5, 2025

Hi @bradymholt,
I can start working on this problem if it is helpful. But I think some parts of the solution might be opinionated, so want to check and make sure we agree on the solution and it's edge cases before I get stuck into it.

General Approach

First off, I'm thinking my general approach would be to create a private function that assess the tsOffset, minute, and hour parts to see if the dateline is crossed. it can return 0 if not crossed, and then -1 or 1 if it is crossed. The solution will assume that -12.00 < tzOffset < +14.00. Then we can use that response to adjust the DOM, DOW, and MONTH parts accordingly.

So for the edge cases:

Day Of Month:

// I am not actually sure how many special cases there are to consider in this category.
// But these are the first few I could think of.

  1. if (DOM === 'L' && dateLineOffset === 1 ) DOM = 'F';
  2. if (DOM === 'L' && dateLineOffset === -1) DOM = 'L-1';
  3. if (DOM === 'L-X' && dateLineOffset !== 0) DOM = L-${X + dateLineOffset}
    // and conversely
  4. if (DOM === 'F' && dateLineOffset === 1 ) DOM = 'F+1';
  5. if (DOM === 'F' && dateLineOffset === -1) DOM = 'L';
    // Day number near the beginning of the Month
  6. if (DOM === 1 && dateLineOffset === -1) DOM = 31
  7. if (DOM === 30 && dateLineOffset === 1) DOM = 1 // or 31?
  8. if (DOM === 31 && dateLineOffset === 1) DOM = 1

Day Of Week

  1. if (DOW === 0 && dateLineOffset === -1) DOW = 6;
    // and conversely
  2. if (DOW === 6 && dateLineOffset === 1) DOW= 0;

MONTH

need to check if the DOM crossed the month line and then increment the month accordingly.
needs to be aware of how many days are in the month for crossing from the L day of the month to the F day of the following month if 28, 30, or 31 is specified rather than L.

Year

Only relevant in the case where year is specified and the DOM and MONTH is set as 1 JAN OR 31 DEC. Which is specific enough that we could just check for those cases and adjust accordingly.

Final Thoughts

Would love to get your thoughts on this and if it is a good starting point. Maybe you are aware of more special cases I have missed. Also not sure how many of them we might want to not handle.
Could also look at handling just the more obvious ones and then splitting the more complex ones into a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants