Sign up to event #104

kristofferlarberg · 2021-03-29T12:48:00Z

Adds possibility for a user to sign up to an event.

Fixes #93

richardolsson

Nice work on this! Definitely a new level of understanding of how things go together.

I have a number of comments though, find them below.

richardolsson · 2021-03-29T17:22:18Z

src/components/EventList.tsx

+                        <Button
+                            data-test="sign-up-button"
+                            marginTop="size-50"
+                            onPress={ () => putEventResponse(e.id, org?.id) }


Great! This is exactly what an event handler should look like.

But why can org be undefined here? That should never be the case.

Perfect! In EventListProps we've previously defined that org can be undefined. I have a vague memory of us leaving it like this since events and hence EventList will never run if we in events don't get orgState?.status === 'success' in getServerSideProps?

If org will never be undefined, it should not be typed as potentially being undefined.

If we know that org will never be undefined we should use non-null assertion or conditionals to indicate that to typescript. Otherwise we're just sweeping the problem in front of us.

This line is essentially saying that if org is undefined, pass undefined to putEventResponse(). What happens if we do that? The function might not work as expected.

Yes, this sounds reasonable. Does the same go for events (non-null assertion or conditionals)?

I don't understand the question. It looks like e is already guaranteed to have a value here, so no changes should be required there.

richardolsson · 2021-03-29T17:23:26Z

src/fetching/putEventResponse.ts

+
+import { ZetkinMembership } from '../types/zetkin';
+
+export default function putEventResponse(event : number, org : number | undefined) : Promise<ZetkinMembership> {


What happens if org is undefined here? Will the code work? I don't think so, and hence undefined should not be an acceptable value for org.

Luckily enough, it's very unlikely that it will ever be undefined, because this function should only be used in cases where an event, and hence the org it belongs to, is known.

See my reply above.

richardolsson · 2021-03-29T17:24:44Z

src/fetching/putEventResponse.ts

+            return eventData;
+        }
+    };
+    return eventSignUp();


Why an inner function that is immediately executed and returned? Doing this is effectively the same as just running the code inside the function, without an inner function.

richardolsson · 2021-03-29T17:25:32Z

src/fetching/putUndoEventResponse.ts

+            if (eventRes.redirected === false) {
+                return null;
+            }


What is the purpose of this?

Since I had a return value on this function I got a parse-related error message from Next when clicking Undo sign-up twice, since this value is erased after the first click. After removing the return value per you comment below this is no longer an issue.

richardolsson · 2021-03-29T17:26:46Z

src/fetching/putUndoEventResponse.ts

+
+import { ZetkinMembership } from '../types/zetkin';
+
+export default function putUndoEventResponse(event : number, org : number | undefined) : Promise<ZetkinMembership> {


Why the return type? Looking at the body of this function, the promise it returns doesn't seem to resolve to a ZetkinMembership.

This same issue exists for the putEventResponse() function (but I can't comment twice on the same line).

richardolsson · 2021-03-29T17:27:25Z

src/fetching/putUndoEventResponse.ts

+            }
+
+            const eventData = await eventRes.json();
+            return eventData;


Why return anything from this function? The only place where this is used (in EventList) doesn't do anything with the return value. That is an indication that there is no need to return anything.

richardolsson

Great progress! Some more comments below. Next up we need to make sure that we can use react-query for this and keep track of the state locally. Have you had any luck in your research?

richardolsson · 2021-03-30T11:17:57Z

src/fetching/deleteEventResponse.ts

+
+import { ZetkinMembership } from '../types/zetkin';
+
+export default async function deleteEventResponse(event : number, org : number | undefined) : Promise<void> {


This is still allowing org to be undefined.

richardolsson · 2021-03-30T11:20:15Z

src/components/EventList.tsx

@@ -36,7 +40,7 @@ const EventList = ({ events, org } : EventListProps) : JSX.Element => {
                        <View data-test="event-title">
                            { e.title ? e.title : e.activity.title }
                        </View>
-                        <View data-test="org-title">{ org?.title }</View>
+                        <View data-test="org-title">{ org!.title }</View>


We've moved the problem up one level. This line means that we claim to know that org is never undefined. If we know as much, then why do we accept undefined in EventListProps?

Either this line is wrong, which I don't think it is, or the props types are wrong.

richardolsson · 2021-03-30T11:21:19Z

src/components/EventList.tsx

+                        <Button
+                            data-test="sign-up-button"
+                            marginTop="size-50"
+                            onPress={ () => putEventResponse(e.id, org!.id) }


Same here as above, i.e. we shouldn't use non-null assertion unless it's inevitable to type something as undefined or null (or any/unknown), even though we know it never can be.

richardolsson · 2021-03-30T11:22:23Z

src/fetching/putEventResponse.ts

+
+import { ZetkinEventResponse, ZetkinMembership } from '../types/zetkin';
+
+export default async function putEventResponse(event : number, org : number | undefined) : Promise<ZetkinEventResponse> {


Same issue here with undefined. Also, if org is just an ID, it should be named orgId (same for deleteEventResponse()).

kristofferlarberg · 2021-03-30T13:57:20Z

Great!

Sorry, I forgot to remove undefined from the prop types. Will do that now!

Regarding react-query that sounds like a good plan. I only have a basic understanding of how mutations work so far, but let’s try!

…rop where EventList is run

richardolsson · 2021-03-30T14:45:01Z

This looks good now! Please move on to preload responses and show only the correct button (either "sign up" or "undo") depending on current state, and update that state when the user interacts.

… button

richardolsson

This is looking good!

As a next step, the code for making API calls should be moved out of EventList, and by "out of", I mean upwards in the tree. I see now that you've moved it down into a separate component ResponseButton.

The problem with this is that it means that EventList (and it's children) now have side effects. They're no longer pure, which makes it much more difficult to test.

The EventList component should not make any network requests itself. It should just take the data that is given to it as props and render UI that reflects that data. If the user clicks a button in the list, it should bubble that up to the surroundings.

I would recommend moving the mutation logic to the container that contains EventList, and adding an event callback function property to EventList, e.g. onEventResponse with three parameters: orgId, eventId and response (boolean).

This will render the <ResponseButton> component useless, so you can remove that again.

You will likely find that these changes mean the same logic needs to be in several places, i.e. every place where EventList is used. If it's a lot of logic, it should be moved into a separate function, e.g. a useEventResponse() hook that returns the onEventResponse function.

kristofferlarberg · 2021-04-09T13:16:16Z

Perfect, this sounds good!

I've been struggling with keeping the requests as far up in the component tree as possible but only made it this far. Your proposal is v helpful! Will start applying this.

…button and mutation

richardolsson

Structure looks great! There are only some minor naming issues, and you are unnecessarily creating a spy even when you're not spying on the function call.

Now all that remains is a custom hook (or any other means) for reusing the mutation logic.

richardolsson · 2021-04-13T16:14:38Z

src/components/EventList.spec.tsx

            });
    });

    it('contains data for each event', () => {
+        const spyOnSubmit = cy.spy();


The naming is off, implies that there is an onSubmit-callback. Is it a rest product from another test?

Also, there should be no need for a spy in this test (or the subsequent ones) where the spy is not actually used for spying.

src/components/EventList.spec.tsx

richardolsson

Nice work. This is very close to something I would be very happy merging! I have a few comments below, please let me know how you would like to proceed.

richardolsson · 2021-04-16T06:14:08Z

src/components/EventList.spec.tsx

+        //Checks for buttons on all events
+        cy.findByText('misc.eventList.signup');


What is the purpose of this?

I see now it's unnecessary since line 81 is already doing the same thing. Will remove this!

richardolsson · 2021-04-16T06:27:12Z

src/pages/o/[orgId]/events.tsx

+    const responseQuery = useQuery('eventResponses', getEventResponses);
+    const eventResponses = responseQuery.data;
+
+    const onEventResponse = useOnEventResponse();


Looking at this again, I think this should all be included in the hook, like I mentioned yesterday. Maybe the hook should even be called useEventResponses() (plural) and be used like so:

const { eventResponses, onEventResponse } = useEventResponses();

Do you feel like you could easily make this work, or should we postpone it to when I can help out? We could merge without it.

Yes I agree! It became very clear when I applied useOnEventResponse() to [eventId] page yesterday. I believe I can manage with this direction. I'll give it a few hours and if it feels too hard I'll let you know and we merge.

richardolsson · 2021-04-16T06:29:13Z

src/types/zetkin.ts

+    id: number;
+}
+
+export interface ZetkinEventSignup{


There is a space missing before {. I'm surprised the linter didn't catch this. Maybe you could add it to the configuration? See space-before-blocks.

We already use space-before-blocks and seems like this is currently not working on interface typescript-eslint/typescript-eslint#1606

richardolsson · 2021-04-16T06:31:10Z

cypress/fixtures/dummyEventResponses.json

+    "data": [
+        {
+            "action_id": 22,
+            "response_date": "1111 11 11, 11:11",


I dislike the use of this very weird date. Why the year 1111, and not something more alike to what would occur in live data? It's also not using the correct ISO format. The API delivers dates like 2021-01-21T18:30:00+00:00, and the dummy data should mimic that.

richardolsson · 2021-04-16T06:32:35Z

cypress/integration/org_events_page.spec.ts

@@ -22,6 +22,20 @@ describe('/o/[orgId]/events', () => {
        cy.visit('/o/1/events');
        cy.get('[data-test="no-events-placeholder"]').should('be.visible');
    });
+
+    it('contains conditional functionality for sign-up button', () => {


A better name would be something like "shows sign up button if not signed up, and undo button when signed up".

richardolsson · 2021-04-16T06:33:00Z

src/components/EventList.spec.tsx

    });

    it('contains a button for more info on each event', () => {
+        const spyOnSubmit = cy.spy();


Why the spy here?

Seems like I missed this when removing spies, will remove.

richardolsson · 2021-04-16T06:33:46Z

src/fetching/deleteEventResponse.ts

+    const mUrl = apiUrl('/users/me/memberships');
+    const mRes = await fetch(mUrl);
+    const mData = await mRes.json();
+    const orgMembership = mData.data.find((m : ZetkinMembership) => m.organization.id === orgId);


This solution is a hack. Maybe leave it like this for now, but add a TODO comment or an issue so we remember to get back to it. It should not be necessary to retrieve the memberships every time. They should be cached.

… and EventList

…/app.zetkin.org into issue-93/sign-up-to-event Pull changes from PR merge

…fetch event responses server side

…eventRepsonses queryClient

richardolsson

Sooo close now! See comments below. 😊

richardolsson · 2021-04-20T10:33:59Z

src/fetching/deleteEventResponse.ts

+function defaultFetch(path : string, init? : RequestInit) {
+    const url = apiUrl(path);
+    return fetch(url, init);
+}


This reoccurs in many locations, and should probably moved to a shared module and reused. Perhaps in src/fetching/index.ts?

richardolsson · 2021-04-20T10:37:10Z

src/fetching/putEventResponse.ts

+    orgId: number;
+}
+
+export default function putEventResponse(fetch = defaultFetch) {


Is fetch injection necessary for this function? Is it ever used on the server?

The same question stands for the delete variant as well.

That's true, it's only used from the client. I'll change this.

richardolsson · 2021-04-20T10:39:11Z

src/utils/next.ts

@@ -74,6 +75,13 @@ export const scaffold = (wrapped : ScaffoldedGetServerSideProps, options? : Scaf
            ctx.z.setTokenData(reqWithSession.session.tokenData);
        }

+        try {
+            ctx.user = await ctx.z.resource('users', 'me').get();


Please extract the actual user data object and store that as ctx.user, not the entire response object returned from get().

richardolsson · 2021-04-20T10:39:29Z

src/utils/next.ts

@@ -74,6 +75,13 @@ export const scaffold = (wrapped : ScaffoldedGetServerSideProps, options? : Scaf
            ctx.z.setTokenData(reqWithSession.session.tokenData);
        }

+        try {
+            ctx.user = await ctx.z.resource('users', 'me').get();


It should be possible to reuse this user object further down in the file, to avoid having to retrieve the user again.

…eleteResponse

richardolsson

Making progress! I feel like we've lost track of the general direction of things and why things were done a certain way to begin with. Take a step back, look at the code as a whole, and if we've removed things that are no longer relevant, any complications introduce to achieve those things should also be removed/simplified again.

I found a few such cases right away below.

richardolsson · 2021-04-20T14:16:50Z

src/utils/next.ts

@@ -104,8 +105,7 @@ export const scaffold = (wrapped : ScaffoldedGetServerSideProps, options? : Scaf
        };

        try {
-            const user = await ctx.z.resource('users', 'me').get();
-            augmentProps(user.data.data as ZetkinUser);
+            augmentProps(ctx.user as ZetkinUser);


This doesn't look right. The try used to be there to verify that the user could be retrieved. If it couldn't, the catch would run augmentProps(null). Now, you are not doing anything here that should raise an exception, so the try is no longer relevant.

The typecast should also not be necessary, as ctx.user should be typed correctly.

True, sorry I missed this, looking at it now it's obvious.

richardolsson · 2021-04-20T14:16:58Z

src/utils/next.ts

@@ -26,7 +26,7 @@ export type ScaffoldedProps = RegularProps & {

 export type ScaffoldedContext = GetServerSidePropsContext & {
    apiFetch: (path : string, init? : RequestInit) => Promise<Response>;
-    user: ZetkinZResult | null;
+    user: unknown;


This should be typed as ZetkinUser.

Yes I tried that, but get the following:

Type 'unknown' is not assignable to type 'ZetkinUser'.

richardolsson · 2021-04-20T14:18:16Z

src/fetching/putEventResponse.ts

 interface MutationVariables {
    eventId: number;
    orgId: number;
 }

-export default function putEventResponse(fetch = defaultFetch) {
+export default function putEventResponse() {


Why return a function in this function? It should not be necessary to wrap the actual operations of this function in an inner function which is then returned. Just run the code in the body of putEventResponse and pass around putEventResponse instead of passing around the return value.

This is a side-effect of the refactoring for server side rendering. But since neither the put or delete functions are run server side I'll revert these back to their previous state (same as you describe).

…ng functions

richardolsson

Great! We made it. 🎉

kristofferlarberg · 2021-04-21T10:17:05Z

Amazing! This feels massive :)

Add sign-up functionality for event

451c9bd

kristofferlarberg requested a review from richardolsson March 29, 2021 12:52

richardolsson requested changes Mar 29, 2021

View reviewed changes

Refactor fetch functions, change name for DELETE function

6c32cd1

kristofferlarberg requested a review from richardolsson March 30, 2021 10:35

richardolsson requested changes Mar 30, 2021

View reviewed changes

Remove undefined from prop types and move non-null assertion to org p…

786eaf8

…rop where EventList is run

kristofferlarberg requested a review from richardolsson March 30, 2021 14:41

Remove eslint-disable from EventList

b19d8df

Update cache when response PUT/DELETE and conditionally render signup…

82da026

… button

richardolsson requested changes Apr 8, 2021

View reviewed changes

Refactor functionality and general structure for conditional sign-up …

2a336c8

…button and mutation

kristofferlarberg requested a review from richardolsson April 13, 2021 11:27

richardolsson requested changes Apr 13, 2021

View reviewed changes

kristofferlarberg added 3 commits April 14, 2021 12:25

Add login procedure in button test

c85776c

Remove unnecessary spies

c83f3fe

Refactor useMutation functionality to reusable hook

db23b2c

kristofferlarberg requested a review from richardolsson April 15, 2021 13:25

richardolsson requested changes Apr 16, 2021

View reviewed changes

kristofferlarberg and others added 8 commits April 16, 2021 10:59

Make small changes in integration and component tests for events page…

f75a42e

… and EventList

Include eventResponses in useOnEventResponse

cdd458a

Add TODO

3dea616

Apply useEventResponses on event page

7d82701

Add space before block

748c496

Merge branch 'main' into issue-93/sign-up-to-event

2f21830

Make integration-tests for events/event pages less flaky

6e5a17d

Merge branch 'issue-93/sign-up-to-event' of https://github.com/zetkin…

2ee9245

…/app.zetkin.org into issue-93/sign-up-to-event Pull changes from PR merge

kristofferlarberg added 3 commits April 19, 2021 16:09

Add user attribute to context

01a5889

Refactor events/event page related fetch functions and conditionally …

0949317

…fetch event responses server side

Adjust integration tests for sign-up button

8970016

kristofferlarberg force-pushed the issue-93/sign-up-to-event branch from f896046 to 8970016 Compare April 20, 2021 08:48

Move user attribute to after reqWithSession, remove else stement for …

3e94a79

…eventRepsonses queryClient

kristofferlarberg requested a review from richardolsson April 20, 2021 09:05

richardolsson mentioned this pull request Apr 20, 2021

Add spy to mock API #123

Closed

richardolsson requested changes Apr 20, 2021

View reviewed changes

kristofferlarberg added 2 commits April 20, 2021 15:53

Make defaultFetch shared module, remove fetch injection in put- and d…

876a5b1

…eleteResponse

Extract user data object as ctx.user and reuse with augmentProps

79a4b05

kristofferlarberg requested a review from richardolsson April 20, 2021 14:08

richardolsson requested changes Apr 20, 2021

View reviewed changes

kristofferlarberg added 2 commits April 21, 2021 11:18

Refactor back putEventResponse and deleteEventResponse to not returni…

6423f59

…ng functions

Add type assertion to user object, remove augmentProps function

8f02e54

kristofferlarberg requested a review from richardolsson April 21, 2021 09:31

richardolsson approved these changes Apr 21, 2021

View reviewed changes

richardolsson merged commit da13605 into main Apr 21, 2021

richardolsson deleted the issue-93/sign-up-to-event branch April 21, 2021 10:04


		import { ZetkinMembership } from '../types/zetkin';

		export default function putEventResponse(event : number, org : number \| undefined) : Promise<ZetkinMembership> {


		import { ZetkinMembership } from '../types/zetkin';

		export default function putUndoEventResponse(event : number, org : number \| undefined) : Promise<ZetkinMembership> {


		import { ZetkinMembership } from '../types/zetkin';

		export default async function deleteEventResponse(event : number, org : number \| undefined) : Promise<void> {


		import { ZetkinEventResponse, ZetkinMembership } from '../types/zetkin';

		export default async function putEventResponse(event : number, org : number \| undefined) : Promise<ZetkinEventResponse> {

		//Checks for buttons on all events
		cy.findByText('misc.eventList.signup');

Sign up to event #104

Sign up to event #104

Conversation

kristofferlarberg commented Mar 29, 2021 • edited

richardolsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristofferlarberg Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristofferlarberg commented Mar 30, 2021 • edited

richardolsson commented Mar 30, 2021

richardolsson left a comment

Choose a reason for hiding this comment

kristofferlarberg commented Apr 9, 2021

richardolsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristofferlarberg Apr 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

kristofferlarberg commented Apr 21, 2021

kristofferlarberg commented Mar 29, 2021 •

edited

kristofferlarberg Mar 30, 2021 •

edited

kristofferlarberg commented Mar 30, 2021 •

edited

kristofferlarberg Apr 16, 2021 •

edited