Fix calendar connection / disconnection in Settings page#1445
Fix calendar connection / disconnection in Settings page#1445davinotdavid merged 6 commits intomainfrom
Conversation
MelissaAutumn
left a comment
There was a problem hiding this comment.
Code looks okay, just need to spin up local appointment to test
| // Find the default calendar's external_connection_id to determine which | ||
| // calendars share the same connection. Calendars sharing the default's | ||
| // connection cannot be disconnected without affecting the default calendar. | ||
| const defaultCalendar = calendars.value?.find( |
There was a problem hiding this comment.
I feel like this needs to be a backend check as well to avoid validation bypass.
There was a problem hiding this comment.
Good call!
Added in this same PR here for completeness:
This commit adds the guards in Google and CalDAV /disconnect routes: 1c36cb7
This commit adds the tests: c0b29fb
(Note to self: a little weird that CalDAV gets the type_id as query param and Google gets it as body 🤔 perhaps a change for another time heh)
MelissaAutumn
left a comment
There was a problem hiding this comment.
Seems to work okay! Can we add another ticket for backend validation protection?
There was a problem hiding this comment.
Thanks Davi, looks great. One question though: What about a CalDAV connection with more than one calendar? E.g. I have a Nextcloud with 2 calendars, both showing up correctly in Appointment. But as soon as I set one of them as default, the other one loses it's 3-dots-menu. Is this intended behaviour or just wrong behaviour in my weird local dev application state? 😅
Other than that, everything in your how-to-test section works perfectly fine on my end. Btw. thanks for always providing perfect PR descriptions!!
|
@devmount thanks for the review and going through so many steps! Yes, that's intentional. In your screenshot, you have two calendars sharing the same External Connection and one of them has been marked as |
devmount
left a comment
There was a problem hiding this comment.
@davinotdavid Perfect, then everything works as expected. Approved!
|
Hey @davinotdavid I'm looking at your E2E tests, I ran them on desktop and mobile. They work on desktop but the mobile tests you added fail on android and iOS. Perhaps you didn't try running your new E2E tests on mobile before merging? The test/e2e/README has instructions on how to run on BrowserStack but feel free to ping me for any help :) Also if you want to add more calendar connections (i.e. so the The mobile connected accounts test updates fail on iOS because playwright doesn't support the BrowserStack link for your updated connected apps test running on iOS here and android here. |
|
@rwood-moz whoops my bad 😅 I will send a PR shortly removing the E2E tests I've added on mobile for now and create a new ticket to update that later on so that we can benefit for the fix now and save some time to focus on the mobile testing after |
Thanks @davinotdavid Yes unfortunately E2E tests on mobile can be quite challenging vs on desktop due to the Playwright framework and BrowserStack not supporting the same methods/features/options on mobile as they do on desktop, and can take a fair amount of time to debug/tailor for mobile. It's really great that you're adding E2E tests though! |
Description of the Change
The
CheckboxInputcomponent from the calendars in the Settings page was still using the@checkedevent handler instead of thev-model. This caused the component not to behave properly so its state was not an accurate reflecting the underlying data of whether or not the calendar was actually connected and furthermore couldn't be toggled at all.We were preventing the default calendar to be disconnected by not showing the dropdown menu options for said calendar because it would cause problems if this calendar is the one set in the Availability. However, we were still allowing other calendars that share the same external connection to be disconnected which is virtually the same since disconnecting a calendar that share the same external connection as the default calendar would be the same as disconnecting the default calendar directly. In this PR, we are now preventing disconnecting calendars that share the same external connection as the default calendar.
How to test
Settingspage and scroll down to see the list of calendarsBenefits
Applicable Issues
Fixes #1441