🐛 Fix: All day event doesn't disappear after changing the week

[murilo9]

This fixes #178

I realised the problem was the fact that it was assumed that an event starting and ending in a different week would be an event that starts in a past week and ends in a future week, wich is not necessarily true. That was making an event that, say, starts and ends in a past week to be rendered as an event that overlaps the current week, which is unitended.

To fix that, I've added a new Category to event.types.ts, called NotAtThisWeek, which is intended for events that starts and ends in a different week, but are not PastToFutureWeek.

I also refactored the tests for getAllDayEventWidth to take into consideration the new logic of pastToFutureWeek and notAtThisWeek. All automated tests have passed.

Obs: this branch was created from my main branch, which was already merged with fix/issue#145. That's why you'll see code unrelated to the current fix. I recommend you to approve this PR first.

[murilo9]

Temporarily closed due to not all tests have passed. I will refactor the tests related to this feature and reopen the PR again when done.

[tyler-dane]

I appreciate you coming up with an approach that works (I tested it). However, I do have some concerns

Dependent Code

This code looks to be branched off #181. The same issues I voiced in that PR apply here. Whenever possible, please avoid making one pending PR dependent on another.

The fix is resource-intensive and has a large code footprint

This implements the fix at the style-level

Here's what I mean by "style-level":
An enum was added to the event category (Category.NotAtThisWeek). This category is used by the getPosition() util, which is used by the GridEvent and AllDayEvent to determine the height/width of the rectangle.

The shortcomings of this include:

1. The NotAtThisWeekisn't currently used in getAllDayEventWidth, so it just falls back to the default value anyway.
2. It further entrenches us in the concept of a 'week', which will make it harder to refactor once we need to support other view durations (1 day, 4 day, 10 day, etc)
3. These position calculations are expensive, so running them only to determine that an event shouldn't be rendered is a waste of the user's compute/memory resources

Suggestion
Instead of fixing the issue at the util/style level, adjust the lifecycle of an event edit. This will let us remove the event automatically, without having to do additional calculations.

Here's how that'd look specifically

Update the submit handler to check whether an event should still be shown based on the latest data

//useDraftUtil.ts
  const submit = (draft: Schema_GridEvent) => {
    const event = prepEvtBeforeSubmit(draft);

   // get this value by comparing the event's dates to 
   // the weekProps.component.startOfView and [...].endOfView
   // (there's already a util that helps with this comparison)
   const shouldRemove = true

    const _payload = {_id: event._id, event};
    const payload = shouldRemove ? {...payload, shouldRemove} : _payload
    const isExisting = event._id;
    if (isExisting) {
      dispatch(
        editEventSlice.actions.request(payload)
      );
      discard();
    } else {
      dispatch(createEventSlice.actions.request(event));
    }
  };

Add a step to the editEvent saga that reacts to the new shouldRemove directive

//event.saga.ts


export function* editEvent({ payload }: Action_EditEvent) {
  try {
    yield put(eventsEntitiesSlice.actions.edit(payload));
    yield call(EventApi.edit, payload._id, payload.event, {
      applyTo: payload.applyTo,
    });

   if (payload.shouldRemove) {
       // remove the id from the week view Redux store:
      // events.getWeekEvents.value.data
   }

    yield put(editEventSlice.actions.success());
  } catch (error) {
    yield put(editEventSlice.actions.error());
    handleError(error as Error);
  }
}

As a result, any all-day event that changed weeks should be removed from the UI immediately after it's removed from the week view Redux slice. This helps us avoid managing that ourselves in util/styling code.

[tyler-dane]

nice catch!

[murilo9]

Understood. I'll discard these changes and start over from a new branch then.

[murilo9]

I'm stuck at the "remove the id from the week view Redux store" part. What do you mean by the "week view" store? I found a getWeekEventsSlice, but it doesn't contain any info about its state (afaik). It's also not clear for me how exactly that would impact the events displayed (or not) in the calendar.

[tyler-dane]

I'm stuck at the "remove the id from the week view Redux store" part. What do you mean by the "week view" store? I found a getWeekEventsSlice, but it doesn't contain any info about its state (afaik). It's also not clear for me how exactly that would impact the events displayed (or not) in the calendar.

Yes the getWeekEvents Redux slice is the one to look at. That's what I was showing in that screenshot above.

The ids in that slice are used to find the corresponding entity in the entity slice. Removing an id from that week slice is an important step to getting this to work, as those ids are what the event selectors are using to decide which events are in view and which arent