5
11
Fork
You've already forked tomato
1

Base event occurrences on shifts instead of events #26

Merged
edwardloveall merged 52 commits from el-common-shift into main 2025年09月24日 19:39:59 +02:00

We had quite a major bug in the app. Some screens wound show that an event occurrence had volunteers or tasks in a list view. But when you would click on it to view or edit it, those details would change. Similarly, volunteers would see different data from organizers. This really caused a lot of trust to be lost in the integrity of our system, and rightfully so!

The core of the problem was that occurrences were generated from events, and events could be pretty easily duplicated. Once there was a duplicate event in the system, trying to generate occurrences from those events was non-deterministic.

Event duplication was happening because their uid (unique identifier) was not actually unique. It was copied over when splitting events.

To fix all this, I'm not collecting events based on their shift. Shift is the key model holding all of these occurrences together. All events and shift_events hang off of a shift. This makes the uid obsolete.

We're still finding buts, but all tests are passing and overall it's been easier to fix those bugs. I've also changed small pieces of the updating code (the code that causes the duplicates) to hopefully not create them in the first place. I have not yet found an algorithm that guarantees that no duplicates will be created.

We had quite a major bug in the app. Some screens wound show that an event occurrence had volunteers or tasks in a list view. But when you would click on it to view or edit it, those details would change. Similarly, volunteers would see different data from organizers. This really caused a lot of trust to be lost in the integrity of our system, and rightfully so! The core of the problem was that occurrences were generated from events, and events could be pretty easily duplicated. Once there was a duplicate event in the system, trying to generate occurrences from those events was non-deterministic. Event duplication was happening because their `uid` (_unique_ identifier) was not actually unique. It was copied over when splitting events. To fix all this, I'm not collecting events based on their shift. Shift is the key model holding all of these occurrences together. All `events` and `shift_events` hang off of a shift. This makes the `uid` obsolete. We're still finding buts, but all tests are passing and overall it's been easier to fix those bugs. I've also changed small pieces of the updating code (the code that causes the duplicates) to hopefully not create them in the first place. I have not yet found an algorithm that guarantees that no duplicates will be created.
@ -2,6 +2,7 @@ class Event < ApplicationRecord
delegated_type :eventable, types: %w[ShiftEvent]
belongs_to :ownable, polymorphic: true
belongs_to :shift_event, foreign_key: :eventable_id
Author
Owner
Copy link

This was to ensure I could use joins or includes. Rails does not support polymorphic associations with those methods otherwise.

This was to ensure I could use `joins` or `includes`. Rails does not support polymorphic associations with those methods otherwise.
@ -0,0 +1,29 @@
class Option
Author
Owner
Copy link

Needing this to represent 3 states:

  • All of something Some([])
  • A specific list of something Some([a, b])
  • None of something None
Needing this to represent 3 states: * All of something `Some([])` * A specific list of something `Some([a, b])` * None of something `None`
@ -7,3 +13,4 @@
return if starts_at.blank? || ends_at.blank?
events = events_from_shifts(shifts:, users:, recurring:)
Author
Owner
Copy link

Since we use shifts now, but we iterate over events, this needs to find the appropriate events to iterate over. This includes events that are assigned to a specific user, events that are not assigned to any user, and events that are both assigned and not assigned (all events).

Since we use shifts now, but we iterate over events, this needs to find the appropriate events to iterate over. This includes events that are assigned to a specific user, events that are not assigned to _any_ user, and events that are both assigned and not assigned (all events).
@ -17,1 +24,3 @@
@occurrence_timeline["#{event.uid}-#{occurrence.starts_at.iso8601}"] = occurrence
shift_id = event.shift_event.shift_id
key = "#{shift_id}-#{occurrence.starts_at.iso8601}"
if @occurrence_timeline.key?(key) && Rails.env.local?
Author
Owner
Copy link

This error has helped me track down a bunch of places where we're creating ambiguous events.

This error has helped me track down a bunch of places where we're creating ambiguous events.
@ -168,0 +184,4 @@
private
def events_from_shifts(shifts:, users:, recurring:)
Author
Owner
Copy link

I don't love these living in here and it's the biggest drawback to me using shift as the main model to find events: you can't filter the events anywhere but inside this timeline. Another iteration could switch back to passing in events, and every caller is responsible for doing the filtering.

I don't love these living in here and it's the biggest drawback to me using shift as the main model to find events: you can't filter the events anywhere but inside this timeline. Another iteration could switch back to passing in events, and every caller is responsible for doing the filtering.
@ -110,3 +106,1 @@
end
EventResult.new(event: this_event)
update(occurrence, eventable_attributes)
Author
Owner
Copy link

I think I'm most proud of this: replacing this with just update. If this ends up working long term, I'll just remove this and replace all the callers with update.

I think I'm most proud of this: replacing this with just `update`. If this ends up working long term, I'll just remove this and replace all the callers with `update`.
thetizzo left a comment
Copy link

I really like the direction this is heading. A huge improvement in clarity to have everything based on the shift instead of an arbitrary uid.

I think, at some point in the future, shift events and events could almost be combined into the same model. It seems like there's a lot of shift.shift_event.event and event.shift_event.shift_id everywhere. Not sure if there's a need to have those to concepts be separate.

I really like the direction this is heading. A huge improvement in clarity to have everything based on the shift instead of an arbitrary `uid`. I think, at some point in the future, shift events and events could almost be combined into the same model. It seems like there's a lot of `shift.shift_event.event` and `event.shift_event.shift_id` everywhere. Not sure if there's a need to have those to concepts be separate.
@ -15,2 +5,2 @@
it "returns an empty array" do
other_organization = create :organization
it "returns an empty array if no shifts exist for that org" do
weekly_event_starts_at = Time.iso8601("2020年10月19日T03:00:00Z")
First-time contributor
Copy link

I think making these lets might help clean this up even further because you could reduce some of the duplication of setup between different tests.

I think making these `let`s might help clean this up even further because you could reduce some of the duplication of setup between different tests.
Author
Owner
Copy link

I'm actually pretty pro-duplication, or at least, I'm pro "put everything that a test needs in that test." I find that using let quickly makes it much harder to work with tests, so as a rule I don't use it. I wrote a bit more about it here if you're curious: https://thoughtbot.com/blog/the-self-contained-test

I'm actually pretty pro-duplication, or at least, I'm pro "put everything that a test needs in that test." I find that using `let` quickly makes it much harder to work with tests, so as a rule I don't use it. I wrote a bit more about it here if you're curious: https://thoughtbot.com/blog/the-self-contained-test
First-time contributor
Copy link

That’s fair. I honestly go back and forth. In this case I thought the de-dup might help because I find reading time strings confounding 😁

I defer to your preference however.

That’s fair. I honestly go back and forth. In this case I thought the de-dup might help because I find reading time strings confounding 😁 I defer to your preference however.
Author
Owner
Copy link

Yeah, I hear ya on those time strings. I'm used to them, but that doesn't mean we can't do better. In tests that I'm writing I tend to use relative dates like 1.day.from_now which I think are easier to read, and are likely to catch unexpected bugs due to their rolling nature.

Yeah, I hear ya on those time strings. I'm used to them, but that doesn't mean we can't do better. In tests that I'm writing I tend to use relative dates like `1.day.from_now` which I think are easier to read, and are likely to catch unexpected bugs due to their rolling nature.
Author
Owner
Copy link

@thetizzo wrote in #26 (comment):

I think, at some point in the future, shift events and events could almost be combined into the same model. It seems like there's a lot of shift.shift_event.event and event.shift_event.shift_id everywhere. Not sure if there's a need to have those to concepts be separate.

Agreed! I've thought the same thing and I may do that one day. For now, it's a little annoying, but mostly harmless.

@thetizzo wrote in https://codeberg.org/rootable/tomato/pulls/26#issuecomment-7324138: > I think, at some point in the future, shift events and events could almost be combined into the same model. It seems like there's a lot of `shift.shift_event.event` and `event.shift_event.shift_id` everywhere. Not sure if there's a need to have those to concepts be separate. Agreed! I've thought the same thing and I may do that one day. For now, it's a little annoying, but mostly harmless.
edwardloveall force-pushed el-common-shift from 8ce642989b
Some checks failed
Setup Successful
Jest Successful
Static Analysis Failed
RSpec Failed
to 72b22d2340
Some checks failed
Setup Successful
Jest Successful
Static Analysis Successful
RSpec Failed
2025年09月24日 17:29:46 +02:00 Compare
edwardloveall force-pushed el-common-shift from 72b22d2340
Some checks failed
Setup Successful
Jest Successful
Static Analysis Successful
RSpec Failed
to e2bc8696df
All checks were successful
Setup Successful
Jest Successful
Static Analysis Successful
RSpec Successful
2025年09月24日 19:29:48 +02:00 Compare
edwardloveall deleted branch el-common-shift 2025年09月24日 19:39:59 +02:00
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
rootable/tomato!26
Reference in a new issue
rootable/tomato
No description provided.
Delete branch "el-common-shift"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?