Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

feat: warn when calendar or calendar event wasn't created #2371

Conversation

gikf
Copy link
Member

@gikf gikf commented Feb 13, 2023

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the main branch of Chapter.

  • Displays basic warning when during chapter and event creation, calendar api didn't create calendar or calendar event.
  • Adds has_calendar_event custom FieldResolver on Event graphql-type.

@gikf gikf added UI/UX This issue deals with UI/UX status: ready for review labels Feb 13, 2023
@gikf gikf requested a review from a team February 13, 2023 20:20
@ghost
Copy link

ghost commented Feb 13, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@ojeytonwilliams
Copy link
Contributor

Hey @gikf, I like the has_calendar_event field, that's much nicer than abusing the ids. My only concern is when to show the warnings.

Warning that the Chapter does not have a calendar seems good. People may forget to authenticate with Google, after all. Also, only instance owners will see this warning and it's reasonable to expect them to have understand how Chapter works.

However, I'm not sure the warning 'Calendar event was not created.' is appropriate unless there's a calendar. If someone chooses not to make use of the integration (due to rate limits, say, #2373) then showing warnings on every event creation is likely to make people think something has gone wrong with Chapter.

If that makes sense, I think the simplest implementation would be to pull useChapterQuery out of the EventForm and into the page components. That's a better separation of concerns, too, since the form shouldn't really have to worry about data fetching.

Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
@gikf
Copy link
Member Author

gikf commented Feb 22, 2023

Good ideas @ojeytonwilliams!

Comment on lines 73 to 79
let hasChapterCalendar = chapterData?.dashboardChapter.has_calendar;
if (chapter_id !== chapterId) {
const { data: currentChapter } = await refetch({
chapterId: chapter_id,
});
hasChapterCalendar = currentChapter.dashboardChapter.has_calendar;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact this is necessary, got me to thinking: why not use the same source of truth for chapter name when on dashboard/chapters/[id]/new-event and dashboard/events/new? Namely admined_chapters.

Reason being, we only care about three things: id, name and has_calendar. If admined_chapters gets has_calendar we don't need to make (let alone pass around) an additional query. As a side benefit, it should be possible to simplify NewEventPage a little.

What do you reckon?

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay getting to this, @gikf

Everything looks great, though, cheers!

@ojeytonwilliams ojeytonwilliams merged commit 9b17716 into freeCodeCamp:main Mar 17, 2023
@gikf gikf deleted the feat/warn-on-no-has_calendar-has_calendar_event branch March 19, 2023 10:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: ready for review UI/UX This issue deals with UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants