hackforla / peopledepot

A project to setup a datastore for people and projects at HackforLA. The link below takes you to the code documentation
https://hackforla.github.io/peopledepot/
GNU General Public License v2.0
7 stars 26 forks source link

Epic: Redesign event schema #88

Open fyliu opened 2 years ago

fyliu commented 2 years ago

Dependency

Overview

As database architects, we would like to make sure the schema is designed well and adhere to good relational design. The current event tables are modeled after the old VRMS non-relational database, and doesn't fit some normal use cases very well as well as having data duplication. We need to redesign it to be simpler, more useful, with no data duplication, and follow the iCalendar (not related to Apple) standard.

Action Items

Resources/Instructions

New Design

New Design

fyliu commented 1 year ago

I meant to be farther along before presenting this, but since you're also looking at this, this is what I have so far.

The recurring event model need fixing.

  1. It does not adequately support common recurring event cases. This problem is easily addressed by adding an rrule field and possibly a couple other fields.

    1. It supports only weekly recurrences, which is most of team meetings, except some that are 2nd Tuesdays, for example.

       RRULE:FREQ=WEEKLY;BYDAY=WE
    2. It doesn't support one-time events, but puts them in a separate table. event vs recurring_event

       RRULE:COUNT=1
    3. It doesn't support weekly except first week of the month CoP events

       RRULE:FREQ=MONTHLY;INTERVAL=1;BYDAY=2WE,3WE,4WE,5WE
    4. It doesn't support monthly CoP leads events

       RRULE:FREQ=MONTHLY;INTERVAL=1;BYDAY=1WE
    5. It doesn't support biweekly alternating weekdays onboarding events (onboarding)

       DTSTART[0]:20230221T122700Z
       RRULE[0]:FREQ=WEEKLY;INTERVAL=2;BYDAY=TU
       DTSTART[1]:20230227T122700Z
       RRULE[1]:FREQ=WEEKLY;INTERVAL=2;BYDAY=MO
  2. Rrule tool

  3. Discussion of how to use rrule and friends

  4. Question about event instance design: is there a strong reason for keeping record of each event instance for recurring events? Seems like a lot of duplication.

    • from the VRMS code: The backend generates any missing events for today every 30 minutes
    • events can be generated on the fly by the client instead of by the backend. Example in javascript
    • the backend can also easily generate individual events in a given date range from an rrule. But this is hiding the real event from the frontend and the event id would be the same for all the individual event instances. There woukd need to be a contract for how to interpret events coming from the server. maybe there's a separate call to get real events vs expanded events
    • I haven't looked at VRMS design docs to see if there's other reasons like being able to add meeting notes for each event. This is the most compelling reason.
  5. Question: How badly do we want to support having a single recurring event to happen Monday or Tuesday for alternating weeks? We can implement this using 2 rrules in one event. "Normal calendar apps like Google and Apple don't allow this, probably because not many people would use it. It's not a lot of work to do. It's not possible to do with a single one. Example for 2 separate events in google calendar

chelseybeck commented 1 year ago

@fyliu I think we can use django-recurrence to simplify this. I'm testing this currently.

Update: I've tested this and it works. There is an error in the installation instructions so I've created this pr in their repo to correct.

fyliu commented 1 year ago

@chelseybeck this looks like exactly what we need. It even handles multiples rrules, which would allow us to have a single event for the slightly complex onboarding schedule, which Google calendar doesn’t support.

Great work finding this! This is what I meant for this issue to do, which is to get rid of all the extra tables and have just the event table doing everything and working better than before. This package does all the heavy lifting for us. Maybe we’ll need another model for overriding single occurrences, to store overwritten fields like different url or different address or changed time. I’m not sure if this package has a recommendation for doing that. Maybe the override part could be designed later if it’s not easy.

That bug is kind of weird since the project says it tests against django 4. The PR may need some kind of guard to work for the several Django versions they support before they will accept it. Or if all their tests passed with the bug, then maybe it needs one more test. Oops, I misread the PR. You're trying to update their docs.

ExperimentsInHonesty commented 1 year ago

Product to make issues for

For reference: Fang's visual comparisons between the current and new table designs. django-recurrence will make it even simpler.

old event tables new event

fyliu commented 1 year ago

Moving this here to clean up Bonnie's original comment above.

  • does DR (Django-recurrence) replace this issue Create Table: cancelled_event #57
  • In a word, NO. But we can make it work. See the next point.
  • Functionally, we don't need a list of cancelled events that #57 would provide. We really need a list of "all" events, including cancelled ones. DR doesn't come with a way to return all events including cancelled ones. It only does the "smart" thing and return all included events minus all excluded ones. Given it provides so much out of the box (models, managers, wrapper code around dateutil.rrule.rrule and rruleset, etc.), I think we still want to keep it rather than implement our own. The way I see it is we have options to give us the cancelled events we need, with tradeoffs.
    1. add the functionality in DR to return all events with cancellation status. There are sub-options in this and we have flexibility to choose one that best fits our needs. This is the place to modify.
    2. we can make the default behavior do what we want, which is to return the unfiltered events list with an added field for whether or not each one is cancelled. This is okay since we're the only one using this modified code. It'll break the existing tests because we're modifying the underlying assumptions about the behaviors of that code. On the up side, it's cleaner when we use it. We just call the same interface and it does exactly what we want. We can fix the broken tests too.
    3. we can leave the default behavior that gives us all the non-cancelled events, and add a separate function to give us only the cancelled events. Code-wise, this is cleaner. It leaves the existing code and tests that go along with them. Interface-wise, This is messier, since it adds a little extra work when we use it, since we would need to combine the lists. Ultimately, it's up to how VRMS wants the data. It could be that they also need just the cancelled events for one of their displays.
    4. add an exdate array (list of excluded dates, meaning cancelled events) outside of DR RecurrenceField and don't set the exdate field inside RecurrenceField. This will make DR give us all the event dates and we manually apply the cancelled dates. This is less good since it means having an extra field, but it means we can use the RecurrenceField without modification. It's also less good if we consider the case where we want to use the exrule (recurrence rule that is an exclude, like NOT on the first Wednesdays of the month)
  • We should discuss these options as a team to see what we think of each one. We should also talk to VRMS about what they need from this.

I created an issue to work on this

fyliu commented 1 year ago
fyliu commented 1 year ago

@Neecolaa and I decided to rename the recurring_event table to event since that's what the new event table is actually doing. I'm adding notes to related issues that should be informed of this redesign.

fyliu commented 1 year ago

Since this is an epic issue, we need to

fyliu commented 5 months ago

I think this needs some more work to create work issues. Moved to New Issues with Draft label.

fyliu commented 1 month ago

I started a thread to compare a new option that showed up recently at

Their differences are not small, and we'd have to use each in a different way.

A GH discussion feels like a good place to hold this information. We can copy a summary of it to the top of this issue later.

fyliu commented 1 month ago

Progress update

  1. Progress:

    • I started looking at django-recurrence and django-recurring.
    • django-recurring is a new package that claims to solve some long-standing django-recurrence problems. But it's implemented differently as a fork.
    • Neither does what we need out of the box.
    • I need to understand them better to determine if either can be modified/used relatively easily to do what we need.
    • If both require extensive work, then we should just implement our own. Maybe we can use inpiration from these packages.
  2. Blockers: It'd be better if I have a clearer set of our Event requirements for evaluating the packages.

  3. Availability: M-Th next week

  4. ETA: Maybe next week

  5. Pictures or links* (if necessary): Discussion link from above.

fyliu commented 3 weeks ago

Status update:

  1. Progress: I didn't make any progress this week
  2. Blockers: None. I got the requirements from last week's meeting.
  3. Availability: M-Th next week
  4. ETA: by next meeting
  5. Pictures or links* (if necessary): Same discussion link.
shmonks commented 2 weeks ago

@fyliu I don't know whether your last status update was from before or after last week's team meeting but just in case, here's the usual update request:

Please provide update

  1. Progress: "What is the current status of your project? What have you completed and what is left to do?"
  2. Blockers: "Difficulties or errors encountered."
  3. Availability: "How much time will you have this week to work on this issue?"
  4. ETA: "When do you expect this issue to be completed?"
  5. Pictures or links* (if necessary): "Add any pictures or links that will help illustrate what you are working on."
fyliu commented 2 weeks ago

Progress: I haven't been able to work on this the past week. I'll make sure to spend time on this this coming week. Blockers: None Availability: M-Th a few hours during the day ETA: Next week

fyliu commented 1 week ago
  1. Progress:
    • I got to the point where I see that recurrence is the best match for what we want, because it has support for exrules.
    • I need to write some code as a proof of concept that we can integrate it.
    • The ultimate goal is to come up with a viable design for the Event model, and that'll take a few steps. I'll update the task list above.
  2. Blockers: None
  3. Availability: M-Th a few hours during the day
  4. ETA: Next week. I should have some code to implement peopledepot's meeting schedule