Repository navigation
refactor: inject the EventKit reads the allowlist depends on - #12
Merged
Merged
Conversation
ReminderService now takes a ReminderStore for the calendar listing, the two reminder predicates, and the fetch. EKEventStore conforms with one wrapper method, and the public init is unchanged. That lets CI exercise the allowlist glue against unsaved EKCalendars, which get real identifiers at creation and need no Reminders access: a stale allowlist builds no predicate (so it can't become EventKit's "all calendars"), a partial one queries only what resolved, and startup validation counts resolved IDs rather than configured ones. Removing either guard fails the new tests.
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The allowlist fix in v2.0.0 shipped untested against EventKit, because
ReminderServicewas never instantiated in a test: every read went straight toEKEventStore, which needs a Reminders permission dialog CI can't click. This moves that boundary behind a small protocol so the glue runs in CI.Change
ReminderStorecovers the five EventKit calls behind reads and the allowlist: the calendar listing, the two reminder predicates, the fetch, and its cancel.EKEventStoreconforms with one wrapper method; the other four already exist on it.ReminderServicegains an internal init taking aReminderStore. The public init is unchanged and passes its ownEKEventStore.calendars(for:)reads, the two predicate calls, and the fetch now go through the protocol.Tests
The stub serves unsaved
EKCalendars, which get real, distinct identifiers at creation without Reminders access, and records the calendar count behind every predicate it builds. That count is what makes the fail-open bug observable: EventKit reads an empty calendar array as "all calendars".Mutation checks. Removing the fail-closed guard breaks two tests (a predicate gets built with 0 calendars). Making validation count configured IDs instead of resolved ones breaks two tests on four assertions. Full suite: 170 tests pass with complete strict-concurrency checking and warnings-as-errors; release build clean.
A first draft of this seam covered only the calendar listing, and its headline test couldn't fail:
getListsreturns[]for a stale allowlist with or without the guard. The mutation check caught that, which is why the seam also covers the predicate and fetch calls.Not covered: the per-reminder checks on update and delete, which need real
EKReminders.ListAccessPolicyalready covers the rule they apply.refactor:title, so this doesn't release.