Add EXDATE and RDATE parsing to to rruleset (TEXT) function

[GavinRay97]

No description provided.

[volkanunsal]

Thanks for the PR! Can you add a couple of tests in this file?

[GavinRay97]

Sure, can try to get to it later this evening or tomorrow 👍

[GavinRay97]

By the way, @volkanunsal thanks for replying so swiftly to this PR. I looked at your activity and saw you hadn't been seen in many months, I was relieved to see you are still around!

I wanted to give background context for why I felt this was important:

The rrule.js library provides .toString() representations of rruleset. But the text input form of the function _rrule.occurrences did not work for EXDATE. You really need EXDATE to handle cancellations/modifications smoothly. The rruleset_to_jsonb function works, but to get it in the correct format, you have to write custom serializers to and from rrule.js data formats when sending it back and forth from the DB:

import { RRule, RRuleSet } from 'rrule'

const FREQUENCIES = [
  'YEARLY',
  'MONTHLY',
  'WEEKLY',
  'DAILY',
  'HOURLY',
  'MINUTELY',
  'SECONDLY'
]
const WEEKDAYS = ['MO', 'TU', 'WE', 'TH', 'FR', 'SA', 'SU']

function rruleSetToPostgresJSON(rruleSet) {
  const [rrule] = rruleSet.rrules()

  let { freq, count, interval, byweekday, dtstart, until, wkst } = rrule.options
  dtstart = dtstart.toISOString()
  wkst = WEEKDAYS[wkst]

  if (freq) freq = FREQUENCIES[freq]
  if (until) until = until.toISOString()
  if (byweekday) byweekday = byweekday.map((weekday) => WEEKDAYS[weekday])

  let exdate = rruleSet.exdates()
  if (exdate.length) exdate = exdate.map((date) => date.toISOString())

  const result = {
    dtstart,
    dtend: until,
    exdate,
    rrule: {
      freq,
      wkst,
      count,
      interval
    }
  }

  return result
}

const serializedRRuleSet = rruleSetToPostgresJSON(rruleSet)

So this is not ideal =/

I'm REALLY poor at SQL, but I was able to follow the pattern that was laid out for parsing the other fields to do EXDATE, and threw RDATE in there because it was exactly the same.

Now, it's possible to store spec-compliant string formats of rruleset objects, without worrying about how a particular library chooses to represent the object/JSON form or doing any serialization 😃

[weyert]

How can we progress this PR?

[GavinRay97]

@weyert

How can we progress this PR?

You could have written the tests he asked about 😛

@volkanunsal I've never worked with SQL much, been spoiled by ORM's my whole life. Can you take a look at the below and tell me if this is roughly what it should look like?

SELECT results_eq(
  $$ SELECT dtstart, rrule, rdate, exdate FROM _rrule.rruleset('
        DTSTART:20120201T023000
        RRULE:FREQ=MONTHLY;COUNT=5
        RDATE:20120701T023000,20120702T023000
        EXDATE:20120601T023000
      ') $$,
  $$ VALUES
    ('2012-02-01 02:30:00'::TIMESTAMP),
    ('(MONTHLY,1,5,,,,,,,,,,,MO)'),
    ('{"2012-07-01 02:30:00","2012-07-02 02:30:00"}'),
    ('{"2012-06-01 02:30:00"}')
  $$,
  'testParseRRuleSetExdateRdate'
);

SELECT results_eq(
  $$ SELECT * FROM occurrences(
      rruleset('
        DTSTART:20120201T023000
        RRULE:FREQ=MONTHLY;COUNT=5
        RDATE:20120701T023000,20120702T023000
        EXDATE:20120601T023000
     ')
  ) $$,
  $$ VALUES
    -- REGULAR OCCURRENCES
    ('2012-02-01 02:30:00'),
    ('2012-02-01 02:30:00'),
    ('2012-03-01 02:30:00'),
    ('2012-04-01 02:30:00'),
    ('2012-05-01 02:30:00'),
    -- EXDATE 20120601T023000
    -- RDATE 20120701T023000, 20120702T023000
    ('2012-07-01 02:30:00'),
    ('2012-07-02 02:30:00')
  $$,
  'testRRuleSetStringOccurrences'
);

[weyert]

Fair point, never written tests for SQL but if you need more tests I can play with it

[volkanunsal]

I ran your tests locally and got these messages:

# Failed test 11: "testParseRRuleSetExdateRdate"
#     Number of columns or their types differ between the queries:
#         have: ("2012-02-01 02:30:00","(MONTHLY,1,5,,,,,,,,,,,MO)",,)
#         want: ("2012-02-01 02:30:00")
# Failed test 12: "testRRuleSetStringOccurrences"
#     Number of columns or their types differ between the queries
# Looks like you failed 2 tests of 12

It'd be good if we showed this in the PR automatically. I'll try to set up a Github action tomorrow to run the tests when you submit a new commit.

[GavinRay97]

Is my syntax wrong for the these test functions? I've not used them before, was just trying to follow the pattern from the others.

This is what I get when I run the queries:

Edit: I see what I broke on the first one, I accidentally duplicated one of the values 🤦‍♀️

-- REGULAR OCCURRENCES
('2012-02-01 02:30:00'),
('2012-02-01 02:30:00'),
('2012-03-01 02:30:00'),

So it should be:

SELECT results_eq(
  $$ SELECT * FROM occurrences(
      rruleset('
        DTSTART:20120201T023000
        RRULE:FREQ=MONTHLY;COUNT=5
        RDATE:20120701T023000,20120702T023000
        EXDATE:20120601T023000
     ')
  ) $$,
  $$ VALUES
    -- REGULAR OCCURRENCES
    ('2012-02-01 02:30:00'),
    ('2012-03-01 02:30:00'),
    ('2012-04-01 02:30:00'),
    ('2012-05-01 02:30:00'),
    -- EXDATE 20120601T023000
    -- RDATE 20120701T023000, 20120702T023000
    ('2012-07-01 02:30:00'),
    ('2012-07-02 02:30:00')
  $$,
  'testRRuleSetStringOccurrences'
);

I am still unsure why the other one fails.

[volkanunsal]

Cool. That looks promising. Can you run the tests locally? Best way to find the problem is to play around with it in your own sandbox. Let me know if you have any issues getting set up to run the tests. I've also added a Github action to run them when you add a commit, so make sure to include your tests in your pull request, as well.

[aliofye]

@GavinRay97 @volkanunsal how can I help to get this PR merged?

[treystout]

Came here for this exact issue, really need EXDATE support at the rulleset level. My understanding is this PR died because nobody wants to write unit tests? Other than that, does the proposed code support properly ignoring occurrences with this code? @GavinRay97 are you using what's presented here? Anything newer?

[volkanunsal]

I'd be happy to merge this PR even without tests. The only issue here is it breaks existing tests. If you can fix those tests, I will merge it.

[TheBrainChain]

Hey @volkanunsal @GavinRay97 , I rebased this and ran the tests. Looks like everything's passing and performing as expected. Any chance we could merge this?