[scheduler] Prevent writing dates without a setter - #23553
mustafajw07 wants to merge 7 commits into
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
|
Hey @rita-codes, can I get a review on this? |
rita-codes
added
type: enhancement
rita-codes
left a comment
rita-codes
left a comment
There was a problem hiding this comment.
Thanks for the PR! The selector, the updateEvent / pasteEvent gates and the tests are all in the right place. Before you go further, one ordering note and then a few changes. I've updated #23499 to match what's below.
Ordering
#23462 changes the premise this PR is built on: after it lands, the event dialog only sends the date fields the user actually edited, and start: undefined in updateEvent means "unchanged". That removes the reason for the instant comparison in removeImmovableDates, and its JSDoc ("the event dialog always submits start / end") stops being true. I'll merge #23462 first. Please rebase on it once it's in, so you only rework this once.
Per-property instead of the pair
The all-or-nothing gate is too coarse. eventModelStructure is declared per property, and the three writers don't need the same thing:
- A drag moves both dates, so it needs both setters.
isDraggablealready did that on master. - A resize changes one date. The
endhandle only needs theendsetter. On masterisResizablewas per side; this PR turns it into the pair. - The dialog edits per field.
isPropertyReadOnlyis per property, so withstartgetter-only andendwith a setter, the dialog rendersendeditable, the user edits it, and the store now discards it.
There is a real use case for that mixed shape: a start fixed by the backend and an editable duration.
What I'd like instead:
- Drop only the date that has no setter, and warn only when that date was an actual change.
isResizablegoes back to per side.isPropertyReadOnlystays as it is.isDraggableand a paste with a new date keep requiring both setters, since those are moves.
Put the guard in updateEvents, not in each public method
updateEvents is the one place every write goes through, and some writers reach it without passing through updateEvent / createEvent / pasteEvent. The recurring scopes are the concrete case: selectRecurringEventScope feeds the plugin's output straight into updateEvents, and "only this" / "this and following" both create a new event with the occurrence's dates. With a getter-only start that new event inherits the source's custom fields, so it doesn't crash, it just lands on the old date silently. The auto-scheduling cascade from #23439 will be another direct caller.
So the two protections should live in updateEvents:
- For
updated: strip from each event the date that has no setter (with the warning above). - For
created: refuse the creation, with a warning, when the structure cannot writestartorend.
The public methods then don't need their own copies of the check. The UI gates (isDraggable, isResizable, creationConfig) stay as the first line so the user never gets to try; updateEvents is the net underneath.
Creation
Same root cause, and this PR is where the rule lives, so let's close it here:
createEventwith a getter-onlystartcurrently writesstartunder the built-in key on a fresh model, the getter returnsundefined, andresolveEventDatethrows a bareTypeErroron the next render.duplicateEventOccurrencedoesn't crash but lands on the old date, same as the copy. Thecreatedguard above covers both.creationConfigshould returnfalsewhenstartorendhas no setter (addeventModelStructureas an input of the memoized selector). That switches off the new-event button, click-to-create and the placeholder at once, the same wayreadOnlydoes.
The scope grows from "prevent moving" to "prevent writing dates without a setter", so please update the title.
Copy / cut
canMoveDates folds in isReadOnly, so pasting a copy of a readOnly: true event now returns null, where master created the copy. A copy never touches the source event, so its readOnly flag shouldn't gate it (Bryntum does the same: cut is gated by the source event, paste by the target scheduler / resource). Keep the readOnly gate for cut; for copy, gate on the scheduler's readOnly and, if you want, the destination resource's areEventsReadOnly.
Smaller things
- Both warnings blame
eventModelStructureeven when the reason isreadOnly. Word them so they name the actual cause. - The Event Timeline events page has the same "Store data in custom properties" section and needs the same paragraph.
- Tests to add: the mixed structure (start getter-only, end with setter, only
endchanged),updateEventon areadOnlyevent, a refused creation throughcreateEventand through a recurring scope, andcreationConfigreturningfalse.
|
Requested changes are done, and the PR has been updated. It’s ready for another review. Thanks! |
rita-codes
left a comment
rita-codes
left a comment
There was a problem hiding this comment.
Thanks for the second pass, this is close. The per-property drop, the per-side resize, the creationConfig gate, the copy / cut split and the requested tests are all there. Moving the created guard into updateEvents introduced one blocking problem though: the guard is not atomic, so a recurring scope edit with a getter-only date now loses data. Details below, then a set of smaller gaps.
Verified locally: the PR's tests pass in jsdom and eslint is clean. #23462 is still open, so the rebase note from the first review still applies; it edits the same region of SchedulerStore.ts, expect a conflict.
Bugs
1. 🔴 Refusing a creation still applies the rest of the batch, so recurring scope edits lose data
SchedulerStore.ts, the created loop in updateEvents:
for (const createdEvent of created) {
if (!canWriteEventDates) {
if (process.env.NODE_ENV !== 'production') { warnOnce([...]); }
continue;
}applyRecurringUpdateOnlyThis returns { created, updated: [{ id, exDates }] } and applyRecurringUpdateFollowing returns { created, updated: [{ id, rrule: { until } }] }; both return { created, deleted: [seriesId] } when no occurrence remains. The updated / deleted half is applied before the loop reaches the refused created.
Reproduced with a scratch test: getter-only start, daily series, edit an occurrence's title in the dialog, pick "only this". onEventsChange receives the series with a new exDates entry and no detached event, so the occurrence vanishes. "This and following" truncates the series with no replacement. "This and following" on the first occurrence calls onEventsChange([]): the whole series is gone.
The editing surface then stays armed on the removed occurrence: in selectRecurringEventScope, movedToEventId is undefined, so the else branch calls setEditingOccurrenceTimes with the never-persisted times and the rrule still set.
Fix: make the batch all-or-nothing. Check canWriteEventDates at the top of updateEvents when created.length > 0 and return early (warning, no onEventsChange, no eventsUpdated, created: []). selectRecurringEventScope should then leave editingOccurrence untouched when createdIds is empty. That also covers point 3.
2. 🟡 "All" scope on the first occurrence realigns the rrule while the start is dropped
applyRecurringUpdateAll computes rrule from changes.start and returns { id, start, end, rrule }. removeUnwritableDates strips only start, so end and the realigned byDay / byMonthDay go through. Weekly Monday series, getter-only start, updateRecurringEvent({ changes: { start: +1 day, end: +1 day } }) then selectRecurringEventScope('all'): the model keeps Monday 09:00 as start, gets Tuesday 10:00 as end and byDay: ["TU"]. Only reachable through the imperative API (drag is disabled and the dialog locks start).
Fix: strip the unwritable dates from changes in selectRecurringEventScope before handing them to the plugin, so it computes with the date absent.
3. 🟡 A fully refused batch still fires onEventsChange and eventsUpdated
Nothing short-circuits when every created item was refused and there is no updated / deleted. onEventsChange is called with the unchanged list, and the lazy-loading plugin calls dataSource.persistEvents({ deleted: [], updated: [], created: [] }) followed by a full events-state rebuild. Covered by the early return in point 1.
4. 🟡 Every end-handle resize of an all-day event warns that start was not updated
const isRealChange =
value == null || adapter.getTime(value) !== original.dataTimezone[property].timestamp;The resize gestures always submit both dates (useDropTarget.ts builds { id: eventId, start, end }), taking the untouched side from displayTimezone. For all-day events displayTimezone.start is normalized to startOfDay while dataTimezone.start keeps the raw instant, so the instants differ and the warning fires on every resize of an all-day event stored as, say, 2025-07-01T09:00:00Z.
The isResizable comment saying a resize commits { id, [side]: value } is therefore not what happens, and the "resize of the end handle alone" test omits start, unlike the real gesture.
Fix: either make the resize handlers submit only the resized side (which also makes the comment true), or compare all-day dates by day instead of by instant. Add a store test that mirrors the real gesture.
5. 🟡 resource: null in a copy paste is treated as "keep the source resource"
const destinationResource = cleanChanges.resource ?? original.modelInBuiltInFormat.resource;null is the documented "no resource" value. ?? sends it to the source's resource, while the created event does get resource: null since stringifiedChanges overrides. Copy an event sitting on a resource with areEventsReadOnly, pasteEvent({ resource: null }): refused with "The destination is read-only" although the destination is "no resource".
Fix: 'resource' in cleanChanges ? cleanChanges.resource : original.modelInBuiltInFormat.resource.
6. 🟡 Cut paste never checks the destination, and a non-moving cut bypasses the scheduler's readOnly
The cut gate runs only inside if (isMovingDates) and checks only isReadOnly(source). Copy checks the destination, cut does not. Not a regression (master had no paste gate), but the comment says cut behaves "like a drag", and a drag on a read-only scheduler is blocked.
- Cut a writable event,
pasteEvent({ start, resource: readOnlyResourceId }): lands on the read-only resource. - Scheduler
readOnly: true,cutEvent(id); pasteEvent({ allDay: true }): applies with no gate.
Fix: gate cut on isResourceReadOnly(destination) as well, regardless of isMovingDates. This is also where canMoveDates would get its consumer (see Simplifications 1).
7. ℹ️ An explicit start: undefined is treated as a real change and warned on
'start' in result is key presence, and value == null sets isRealChange. updateEvent({ id, title, start: undefined }) warns although no date was requested. Harmless today; once #23462 lands and untouched dates are omitted, revisit this branch and the JSDoc that depends on the dialog always submitting both dates.
8. ℹ️ createEvent, duplicateEventOccurrence and a copy paste can return undefined typed as SchedulerEventId
.created[0] is undefined on refusal but inferred as SchedulerEventId; pasteEvent is declared SchedulerEventId | null and now also returns undefined. No in-repo caller uses the value today. Widen the return types or return null consistently.
9. ℹ️ creationConfig is memoized on the structure reference, not on the boolean it derives
An inline eventModelStructure prop is a new reference every render, so the memo now returns a fresh config object each time and every useStore(creationConfig) subscriber re-renders. Keying the input on canWriteEventDatesSelector(state.eventModelStructure) keeps it stable.
Tests
- 🔴 The "only this" refusal test asserts only
eventIdList.length, which anexDatesupdate never changes, so it cannot see point 1. Assert theonEventsChangepayload (seriesexDatesandrruleuntouched, or no call at all once fixed), and add "this and following" plus thedeletedbranch (a single-occurrence series, where the length does drop). - 🟡
endgetter-only never goes through the store: onlyreadOnlyStartStructureis used. Theendwarning text, the mirror case (writablestart, droppedend) and both dates getter-only in one update are untested. - 🟡
duplicateEventOccurrenceunder a getter-only structure is untested. It is the third public writer thecreatedguard covers. - 🟡 The destination-resource copy test uses
READ_ONLY_EVENTas the source, so the refusal could come from either gate. Use a writable event, and add the?? original.resourcefallback case (copy of an event already on a read-only resource, pasted withoutresource). - 🟡 The "only touches the writable date" resize case omits
start; the real handlers send both dates. A test with both dates and an all-day event would have caught point 4. - ℹ️ Weak assertions: the two copy refusals do not assert
copiedEventis retained (the cut one does); the read-onlyupdateEventtest asserts neither.not.toWarnDev()nor the call count; thecreateEventrefusal assertslastCall[0]deep-equals[], which is also true for an initial empty call.isDraggablewith the mixed structure and cut onto a read-only destination are untested. - ℹ️
UNCHANGED_EVENT/READ_ONLY_EVENTare object fixtures; we use camelCase for those and UPPER_CASE for primitives. Three per-describecreateStorehelpers with different signatures in one file could be one file-level helper taking params.
Simplifications
- 🟡
canMoveDatesis exported with no consumer: the only call is insideisDraggable, andpasteEventre-derives the same predicate by hand while the JSDoc claims paste uses it. Either make the cut path call it, or keep it private and drop the export plus itsdescribe, which only re-tests the composition ofisReadOnlyandcanWriteEventDates. - 🟡 Three paste warning blocks share the same first line,
missingSetterWarningis built on every paste, and two 12-line ternaries repeat the "was not pasted" line. A localrefuse(reason)closure that warns and returnsnullremoves about 25 lines, and the missing-setter sentence can be one module constant shared with thecreatedguard. - 🟡 Comment volume. The
removeUnwritableDatesJSDoc is 10 lines and ends with speculation about a future auto-scheduling cascade. The "no old date to fall back to" rationale appears five times (store, selector comment, selector JSDoc, selector test, docs). The cut / copy blocks are 4 lines each,canWriteEventDatescarries the same JSDoc twice, and several test preambles read as PR history ("this is the fix: master created the copy"). One plain sentence each is enough. - ℹ️
removeUnwritableDates:let resultplus a rest-destructure per property leaves a deadremovedDatebinding. One shallow copy anddelete result[property]is equivalent.adapter.isEqualis the existing idiom for date comparison in this file. - ℹ️
isResourceReadOnlytakes a resource id and mirrors theeventColorpattern inschedulerResourceSelectors, so it belongs there.isMovingDatescould be hoisted above theendderivation, which already tests the same condition.
Docs
- 🟡 "A copied event can never be pasted, since a copy always writes both dates into a new event." opens a new paragraph, so the "When
startorendis read-only" scope is lost and it reads as unconditional. Say "When either date is read-only, a copied event can't be pasted at all, since a copy writes both dates into a new event." - 🟡 The paragraph documents APIs that are not public.
copyEvent/cutEvent/pasteEventhave no caller outside the store, and the editing page says "Copy and paste events 🚧 — This feature isn't available yet".updateEvent()is not onbuildPublicAPI(). Keep the user-facing part: dragging disabled, the other resize handle stays enabled, the dialog leaves that date unchanged, creation disabled. Same on the Timeline page. - 🟡 The two read-only warnings ("The event is read-only, so it cannot be moved to a new date." / "The destination is read-only, so a copy cannot be created there.") stop at the why. Add what to remove:
readOnlyon the event,areEventsReadOnlyon the resource, or the scheduler prop. - 🟡 The
removeUnwritableDatesJSDoc says "the event dialog always submitsstart/end, even when it rendered those fields locked". True today, false after #23462. Word it without the dialog: only the unwritable date is dropped, the rest applies. - 🟡 The title still says "prevent moving events"; the scope is now update, creation, copy / paste and
creationConfig. Please retitle and add a changelog entry. - ℹ️ "read-only" now has two meanings on the page:
readOnly: true(UI gate) further up, and getter-only properties (store-level drop) in the new paragraph. "Getter-only" or "not writable" for the new one avoids readers applying the editing page's precedence rules.
|
Changes are done. Please take a look and let me know if anything else needs updating. |
mustafajw07
changed the title
2 participants
Closes #23499
Changelog
Events with non-writable start or end dates can no longer be moved or created with those dates. The scheduler now preserves non-writable dates when updating events, disables creation when required date setters are unavailable, and prevents invalid date changes through resizing and copy/paste operations.