feat: integration info during event creation #2292
Conversation
👇 Click on the image for a new way to code review
Legend |
| return { | ||
| Icon: InfoIcon, | ||
| text: 'Instance is authenticated with calendar api, and chapter has created calendar. Chapter will attempt creating event in the calendar.', | ||
| }; |
There was a problem hiding this comment.
It doesn't seem necessary to warn people that things are going to work as expected
There was a problem hiding this comment.
I wanted to include it as a part if something goes wrong, as there isn't clear path to inform client when creating calendar event errors.
This got me thinking, what if creating calendar and calendar event is pulled out of the create chapter/event resolvers? The actual creation can be then run once entry is created. That gives ability to separate and inform about errors on both these steps:
- Create chapter/event. In case of errors, stop and inform client. Otherwise go to next step.
- From client run mutation creating calendar/ calendar event. Errors can be clearly indicated as coming from calendar integration. They could be also displayed as warnings.
Edit: Or keep it in resolver and just check in object returned by mutation 🤔
There was a problem hiding this comment.
I like the sound of that.
Potentially the Google Event creation could fail in a bunch of ways (lack of permissions, rate limits, network error, bad configuration etc. etc.), so rather than trying to warn organisers that the Event will not get created we should focus on handling the errors when it inevitably does fail. Particularly since there are no checks we can carry out that will guarantee that an Event will be created - only that it has been.
Even if the owner has forgotten to authenticate with Google, it should only be a problem for the first event. After which they'll see the warning, authenticate and create the calendar event (once that's implemented).
What do you reckon?
There was a problem hiding this comment.
That seems to be better way of handling it. I'll block this PR for now, start with simple informing if calendar/event in calendar was created, and see how to go from there.
There was a problem hiding this comment.
Cool, that sounds like the way to go.
| } | ||
| return { | ||
| Icon: WarningTwoIcon, | ||
| text: "Instance is authenticated with calendar api, but chapter doesn't have calendar created. Event will not be created in the calendar.", |
There was a problem hiding this comment.
How about the following?
"This chapter does not have a calendar. To allow calendar event creation, go to link-to-chapter and create a calendar"
| const isBroken = isAuthenticated === null; | ||
| if (isBroken) { | ||
| if (hasCalendar) { | ||
| return { | ||
| Icon: WarningTwoIcon, | ||
| text: 'Chapter has calendar created, but calendar integration is not working. Event will not be created in the calendar.', | ||
| }; | ||
| } | ||
| return { | ||
| Icon: WarningTwoIcon, | ||
| text: 'Calendar integration is not working, and chapter does not have calendar created. Event will not be created in calendar.', | ||
| }; | ||
| } | ||
| return { | ||
| Icon: WarningTwoIcon, | ||
| text: 'Instance is not authenticated with calendar api. Automatic creation of events in calendar is not possible.', | ||
| }; |
There was a problem hiding this comment.
Do we need to distinguish between these three cases? I think it makes sense to handle hasCalendar separately, but if Chapter's tokens are missing or invalid, the fix is simply to reauthenticate.
With that in mind, how about this
Chapter is not authenticated with Google Calendar. Go to
link to calendar dashto authenticate.
Chapter is not authenticated with Google Calendar and this chapter does not have a calendar. Go to
link to calendar dashto authenticate. If you wish to create a calendar, visit the chapter dashboard.
There was a problem hiding this comment.
I'm a bit assuming that person seeing these warnings not necessarily has privileges to (re-)authenticate. Otherwise the alert on the top would be catching the attention to re-authenticate.
|
Since we never quite resolved this one, I'll have to close it. |
Update README.md).mainbranch of Chapter.dashboardChapterwas needed to get thehas_calendar.