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

Epic: Redesign event schema #88

Open
12 of 14 tasks
fyliu opened this issue Sep 27, 2022 · 9 comments
Open
12 of 14 tasks

Epic: Redesign event schema #88

fyliu opened this issue Sep 27, 2022 · 9 comments
Labels
complexity: missing draft This issue is not fully-written epic Issue is an epic Feature: DB Design p-feature: events ready for dev lead research Issue involving doing research s: CTJ stakeholder: Civic Tech Jobs s: hackforla.org stakeholder: hackforla.org website s: PD team stakeholder: People Depot Team s: VRMS stakeholder: VRMS size: 3pt Can be done in 13-18 hours

Comments

@fyliu
Copy link
Member

fyliu commented Sep 27, 2022

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

  • rrule demo

  • search for "data model for recurring calendar events"

  • stackoverflow answer - other answers could be useful as well

  • iCalendar standard RFC 5545

  • Tim Shaker's schema is also worth referencing. It is modeled after google calendar

  • Why it needs to be changed

    • it's unnecessarily complex
    • it only has weekly recurrence but no monthly like the CoP Leads meetings
    • each recurring event instance doesn't need to generate a new database record, unless there's design considerations for doing so
    • use the iCalendar standard rrule to define recurrence in a single field (replaces day_of_week, frequency_id (interval), fields missing(start_datetime, end_datetime, timezone, yearly, monthly, and more)
    • unnecessary data duplication with location data fields. Eliminate event/event_history tables location redundancies #55

New Design

  • It's mostly the same as the old one except the 3 core event tables are combined into one and the recurrences field is going to take care of the functionalities that were spread across the 3 tables. Some tables were dropped because the recurrences field also takes care of their functions.
  • To edit the diagram, copy the link and replace png with uml.

New Design

@fyliu
Copy link
Member Author

fyliu commented Feb 19, 2023

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
Copy link
Member

chelseybeck commented Mar 4, 2023

@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
Copy link
Member Author

fyliu commented Mar 5, 2023

@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 ExperimentsInHonesty added s: PD team stakeholder: People Depot Team s: CTJ stakeholder: Civic Tech Jobs s: VRMS stakeholder: VRMS s: hackforla.org stakeholder: hackforla.org website labels Mar 5, 2023
@ExperimentsInHonesty
Copy link
Member

ExperimentsInHonesty commented Mar 31, 2023

Product to make issues for

  • eliminate recurring table because we are going to use the RecurrenceField provided by django-recurrence (DR)
  • does DR replace this issue Create Table: cancelled_event #57
  • writing up the fields needed for event table
    • db architect will gather all the fields from all the tables and fields that are being eliminated so that Chelsey can identify which any of of them are still needed when using django recurrence
    • RecurrenceField (required)

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
Copy link
Member Author

fyliu commented Apr 14, 2023

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

  • 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 Create Table: cancelled_event #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.
      1. 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.
      2. 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.
    2. 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
Copy link
Member Author

fyliu commented Aug 4, 2023

  • write down the final table design here. Added to the issue description.
  • modify the ERD
  • modify the spreadsheet

@fyliu
Copy link
Member Author

fyliu commented Aug 11, 2023

@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.

  • We need to pause this issue until Update Table: recurring_event (rename to event) #173 is done. It will do the rename, but it contains changes that we can make before tackling this one. This is probably the better path even though it's going to change the same model twice.

@fyliu fyliu removed their assignment Aug 11, 2023
@fyliu fyliu changed the title Redesign event schema Epic: Redesign event schema Aug 14, 2023
@fyliu
Copy link
Member Author

fyliu commented Aug 14, 2023

Since this is an epic issue, we need to

  • create a work issue to update the event model.

@fyliu
Copy link
Member Author

fyliu commented Jun 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity: missing draft This issue is not fully-written epic Issue is an epic Feature: DB Design p-feature: events ready for dev lead research Issue involving doing research s: CTJ stakeholder: Civic Tech Jobs s: hackforla.org stakeholder: hackforla.org website s: PD team stakeholder: People Depot Team s: VRMS stakeholder: VRMS size: 3pt Can be done in 13-18 hours
Projects
Status: New Issue Review
Development

No branches or pull requests

3 participants