Events part 6: edit event page #1862

darrenvong · 2021-08-23T03:45:44Z

This is probably the final piece for the MVP of #541. It looks more or less identical to the "create event page" since they share the same form UI. This has been intentional with how I put the form UI into its own component:

Also added a new "edit event" button that's only visible to users with edit or community moderation permissions:

Frontend checklist

Formatted my code with yarn format && yarn lint --fix
There are no warnings from yarn lint
There are no console warnings when running the app
Added any new components to storybook
Added tests where relevant
All tests pass
Clicked around my changes running locally and it works
Checked Desktop, Mobile and Tablet screen sizes

In preparation for it to be re-used in edit event page

Create event form will largely remain unaware of this change Also did a tiny refactor to make the submit button area more flexible, in order to decouple the event form UI from messy logic to determine whether to show the create or update variant and defers that responsibility back to the parent page instead.

Fixed edge case where event value is not populating form if user visits the page directly by delaying rendering of form until the query has loaded. This is because the event wouldn't have been in cache yet if user goes to the edit event form directly, and so form would use an undefined event for first render (as if it's creating a new one).

The TZ=UTC flag might have broken it? Anyway I think it's a good introduction to stop us worrying about timezone differences between our computer and CI.

darrenvong · 2021-08-23T23:43:28Z

app/frontend/package.json

@@ -33,11 +33,11 @@
  "scripts": {
    "start": "react-scripts start",
    "build": "CI=true react-scripts build",
-    "test": "react-scripts test",
+    "test": "TZ=UTC react-scripts test",


Wish I found this out earlier for ensuring the timezone used is the same across our computer and CI. The timezone mock thing has been causing me pain for a while as it'd error with something about unknown Date constructor as soon as I need to interact with the date inputs/dayjs.

This seems like the only solution that works by changing the test timezone to UTC with fake timers to "fix" the current date so our test data would work/pass date validation etc

thdk · 2021-08-25T06:46:26Z

app/frontend/src/features/communities/events/EventForm.test.tsx

+    jest.useFakeTimers("modern");
+    jest.setSystemTime(new Date("2021-08-01 00:00"));


Move to beforeAll on line 211? Else adding a second test in this describe method will still have this system date. Or not yet have this system date if the second test was executed before this test that sets the system date.

This is specifically placed here (after the location click from LocationAutocomplete on line 225-226) since it has the same problem as the other test - that the msw server doesn't respond when the fake timers are on. I probably could leave a comment here too.

As for the concern that this will break things if we add more tests here, I could move the jest.useRealTimers() to an afterEach instead so it will correctly reset the timer after every test

app/frontend/src/features/communities/events/CreateEventPage.test.tsx

lucaslcode · 2021-08-25T18:51:32Z

app/frontend/src/features/communities/events/CreateEventPage.tsx

+      {({ isMutationLoading }) => (
+        <>
+          <Button
+            className={classes.submitButton}
+            loading={isMutationLoading}
+            type="submit"
+          >
+            {CREATE}
+          </Button>
+          <Typography className={classes.disclaimer} variant="body1">
+            {CREATE_EVENT_DISCLAIMER}
+          </Typography>
+        </>
+      )}
+    </EventForm>


Not a big deal but to me this seems more opaque than a required submitButton prop.

I thought about using an explicitly named prop than using children, but since this is literally appearing at the bottom of the form, it fits naturally enough with composition for me in this case.

lucaslcode

Changing an event from offline to online doesn't move it to the global community.

Also, there's a backend bug - changing something other than the date gives an error about not being allowed to have overlapping occurrences.

darrenvong · 2021-08-25T19:10:49Z

Changing an event from offline to online doesn't move it to the global community.

I think that's also a backend flaw. Currently UpdateEventReq doesn't allow a parentCommunityId to be specified, so the API will need to be updated to allow that too. Otherwise the current workaround would be an additional API call to TransferEvent

darrenvong · 2021-08-26T01:36:54Z

Also moving the event from online to offline to a different community (going from what you said) is also technically impossible at the moment, as all events will originate from the global community in those cases and there's no way for the frontend to figure out which community a location belongs to... but I guess that logic's kinda broken anyway since we are allowing events to be created in locations that might not match with where the community say it is.

So maybe it doesn't make much sense to move an event to the global community because it's going from offline to online? Or are we allowing it anyway as a non-reversible thing?

darrenvong requested a review from a team August 23, 2021 03:46

darrenvong added 2.has feature (new) Implementation of a feature 1.topic frontend labels Aug 23, 2021

darrenvong added 7 commits August 23, 2021 04:52

feat: add updateEvent to service layer

1714f2c

feat: add and link edit event form from single event page

4cf064e

refactor: fetching single event into hook

42b481a

In preparation for it to be re-used in edit event page

feat: pass event through to event form in edit event page

56675da

feat: add tests for event form for when an event is passed to it

2b71271

feat: test to check edit event button rendering logic

7f5c21e

darrenvong force-pushed the frontend/feature/edit-event-page branch from 234bd80 to 7f5c21e Compare August 23, 2021 03:53

darrenvong force-pushed the frontend/feature/edit-event-page branch from 8ff7df0 to 54a5a4c Compare August 23, 2021 23:19

darrenvong marked this pull request as ready for review August 23, 2021 23:19

fix: broken create event page tests

2bb1ecd

The TZ=UTC flag might have broken it? Anyway I think it's a good introduction to stop us worrying about timezone differences between our computer and CI.

darrenvong force-pushed the frontend/feature/edit-event-page branch from b34521b to 2bb1ecd Compare August 23, 2021 23:46

feat: add event load error state for edit event page/form

cbe48f9

darrenvong commented Aug 24, 2021

View reviewed changes

thdk reviewed Aug 25, 2021

View reviewed changes

lucaslcode reviewed Aug 25, 2021

View reviewed changes

lucaslcode requested changes Aug 25, 2021

View reviewed changes

lucaslcode mentioned this pull request Aug 25, 2021

Editing event with similar time gives "overlapping occurances not allowed" error #1869

Closed

fix: test tidies from PR feedback

aaf71bc

darrenvong requested review from lucaslcode and thdk August 27, 2021 16:22

lucaslcode approved these changes Aug 29, 2021

View reviewed changes

darrenvong merged commit b6bf022 into develop Aug 29, 2021

darrenvong deleted the frontend/feature/edit-event-page branch August 29, 2021 21:58

darrenvong added the release notes: pending Add to stuff that should be included in release notes label Aug 29, 2021

darrenvong mentioned this pull request Aug 29, 2021

Events MVP #541

Closed

8 tasks

aapeliv added release notes: done Was mentioned in release notes and removed release notes: pending Add to stuff that should be included in release notes labels Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Events part 6: edit event page #1862

Events part 6: edit event page #1862

darrenvong commented Aug 23, 2021 •

edited

Loading

darrenvong Aug 23, 2021

thdk Aug 25, 2021

darrenvong Aug 25, 2021

lucaslcode Aug 25, 2021

darrenvong Aug 25, 2021

lucaslcode left a comment

darrenvong commented Aug 25, 2021

darrenvong commented Aug 26, 2021

		jest.useFakeTimers("modern");
		jest.setSystemTime(new Date("2021-08-01 00:00"));

Events part 6: edit event page #1862

Events part 6: edit event page #1862

Conversation

darrenvong commented Aug 23, 2021 • edited Loading

darrenvong Aug 23, 2021

Choose a reason for hiding this comment

thdk Aug 25, 2021

Choose a reason for hiding this comment

darrenvong Aug 25, 2021

Choose a reason for hiding this comment

lucaslcode Aug 25, 2021

Choose a reason for hiding this comment

darrenvong Aug 25, 2021

Choose a reason for hiding this comment

lucaslcode left a comment

Choose a reason for hiding this comment

darrenvong commented Aug 25, 2021

darrenvong commented Aug 26, 2021

darrenvong commented Aug 23, 2021 •

edited

Loading