7 users can register for updates on the holon development #13

broekhuisg · 2022-07-05T06:32:09Z

Backend in Django.
localhost:8000 om te bereiken. /swagger/ om de API/Swagger te bekijken.

Endpoint update-register, kan je alleen naar POSTen als anonymous user. (om de registraties voor de updates in op te slaan)

Via /admin (waar je dan nog wel even een superuser voor moet aanmaken - zie readme - kan je alle registraties zien).

Door niet de hele modelviewset vanuit rest framework in te laden maar alleen de GenericViewSet en de CreateModelMixin kun je afvangen dat alleen een post mogelijk is zonder zelf permissions te moeten schrijven.

mattijsstam

@broekhuisg belangrijkste opmerking is het ontbreken van een api om alleen een emoticon als reactie te geven, verder heb ik een suggestie gedaan hoe je rest framework flexibeler kunt gebruiken.

mattijsstam · 2022-07-05T07:48:30Z

backend/backend/urls.py

-    path('admin', admin.site.urls)
+    path("", include(router.urls)),
+    path("admin/", admin.site.urls),
+    re_path(


Het is beter om onderstaande url's alleen in development in te laden en niet in productie

mattijsstam · 2022-07-05T07:52:49Z

backend/holon/models/update_register.py

+    Model describing an User Registration
+    """
+
+    RATING_CHOICES = (


We hadden tijdens de refinement besproken dat het ook mogelijk moet zijn om alleen een emoticon als reactie te geven zonder details. Dit is in bovenstaande api niet mogelijk, je kan het opsplitsen in 2 endpoints of in 1 api afvangen. Je verliest dan wel de link tussen de rating en de contactgegevens @thesethtruth is dit een probleem?

No Link between emoticons and registration

mattijsstam · 2022-07-05T07:58:49Z

backend/holon/permissions.py

@@ -0,0 +1,11 @@
+# permissions.py


Dit bestand is dus waarschijnlijk overbodig

antw · 2022-07-05T10:45:20Z

@mattijsstam @broekhuisg I have pushed the front end.

I haven't been able to test properly with the back end: the Cross-Origin Resource Sharing settings in the back end are too strict and requests are being denied with 403 Forbidden. I can't find any references to CORS or CorsMiddleware in the Django app itself, so presumably the defaults are too restrictive? Can they be relaxed to allow API requests from any origin?

I've added the contact form which will submit a POST to {BACKEND}/update-registrations/. I looked at the Django app to figure out what was needed; it's configured to send the following JSON payload (please let me know if this is incorrect):

interface RegistrationPayload {
  name: string;
  email: string;
  company: string | undefined;
}

The sentiment / ratings is configured to send a POST to {BACKEND}/update-ratings/ with JSON:

interface SentimentPayload {
  rating: "heart" | "thumbsup" | "neutral" | "thumbsdown"
}

Again, please let me know if this is incorrect. If you wish to change the name of the endpoints, you may do so here:

I'm relying on the browser validation for name and e-mail fields being required. If the Django app will also return validation or error messages, let me know and I'll update the components to display these.

Feedback is very welcome!

- Adds a feedback form allowing visitors to enter their name and e-mail, and optionally their company. Data will be submitted to an API (pending). - Adds a Sentiment component which allows visitors to select how they feed about the page. - Moves EmoticonButton into the Sentiment module since it's unlikely to be used elsewhere. Ideally, I think I'd like to lift the calls to `fetch` out to a parent component for both Sentiment and SubscriptionForm, but for now this feels like a nice and simple way of doing what we need.

- Uses yarn to run the app in development mode. - node_modules are stored in a volume.

…nt' of github.com:ZEnMo/Holon-webapp into 7-users-can-register-for-updates-on-the-holon-development

mattijsstam

@antw please see the remark in the SubscriptionForm.js file

mattijsstam · 2022-07-05T14:13:51Z

frontend/components/FeedbackBlock/SubscriptionForm.js

+      body: JSON.stringify(payload),
+    })
+      .then(() => dispatch({ type: "resolve" }))
+      .catch(() => dispatch({ type: "reject" }));


@antw The following does not work because fetch resolves if the api sends a 400 or 404 or because it is technically an ok response. Please see the following article for an example on how to catch server errors with fetch.

Also on reject the error that is send by the server is not shown to the user, for example this form can be submitted but does return an error which is not shown.

@antw Within Witteveen+Bos we always use axios because it has a more user friendly api and prevents this sort of (to me) illogical things.

I noted in my previous comment that the form wasn't handling any server-side validation errors yet; I didn't have Django responses working so it wasn't clear what the errors would look like. I'll revisit this ASAP to add support for this.

I appreciate the reminder about fetch and 4xx responses though. 🤦

@broekhuisg I took care of it already in 61e2d83. Personally, I don't think it's worth adding a dependency just for nicer handling of 4xx errors; they'll be pretty uncommon given that browsers will catch most validation errors already.

@antw I see that you already fixed this issue. Thanks. I would have suggest to use the package Axios, but this is good for now I think :)

thesethtruth · 2022-07-05T17:39:42Z

A slight remark from my side: I noticed that the backend only uses a timestamp to store the sentiment/rating. I checked the frontend and a user is able to trigger multiple posts to the backend from there. Ideally, that would just update the given sentiment. But with the current implementation that just creates a new entry, right?. Would it be possible to use a session ID that is used to identify the current session? If it already exists, the entry is updated.

mattijsstam · 2022-07-06T05:21:04Z

A slight remark from my side: I noticed that the backend only uses a timestamp to store the sentiment/rating. I checked the frontend and a user is able to trigger multiple posts to the backend from there. Ideally, that would just update the given sentiment. But with the current implementation that just creates a new entry, right?. Would it be possible to use a session ID that is used to identify the current session? If it already exists, the entry is updated.

@thesethtruth I think it's best to create a new issue with this change and try to work out some technical implications on this issue. This way we can merge this one.

broekhuisg · 2022-07-06T08:11:23Z

A slight remark from my side: I noticed that the backend only uses a timestamp to store the sentiment/rating. I checked the frontend and a user is able to trigger multiple posts to the backend from there. Ideally, that would just update the given sentiment. But with the current implementation that just creates a new entry, right?. Would it be possible to use a session ID that is used to identify the current session? If it already exists, the entry is updated.

@thesethtruth @mattijsstam I will resolve this issue. I will set a localStorage item with the value the user has set. If these item exists, it will set in the frontend and will by change do a PUT request. Otherwise POST request.

broekhuisg · 2022-07-06T11:25:54Z

A slight remark from my side: I noticed that the backend only uses a timestamp to store the sentiment/rating. I checked the frontend and a user is able to trigger multiple posts to the backend from there. Ideally, that would just update the given sentiment. But with the current implementation that just creates a new entry, right?. Would it be possible to use a session ID that is used to identify the current session? If it already exists, the entry is updated.

I fixed your question about the PUT/POST request. With the remark that it is now possible for people to edit ratings when they are guessing the ID of a rating. So this is a potential risk/leak. But since we are dealing with a prototype and we are not using any form of login, I think we can use this for now. @thesethtruth @mattijsstam

thesethtruth · 2022-07-06T12:31:52Z

I fixed your question about the PUT/POST request. With the remark that it is now possible for people to edit ratings when they are guessing the ID of a rating. So this is a potential risk/leak. But since we are dealing with a prototype and we are not using any form of login, I think we can use this for now. @thesethtruth @mattijsstam

Awesome, thanks good fix for now! How do we track technical debt, @mattijsstam?

Fixes that the errors object would be set to a promise rather than the errors JSON.

antw

Looks good to me. I think this can be merged. 👍

I tested adding a couple of registrations and ratings, and everything worked as I expected.

@mattijsstam I cannot reproduce the styling issues seen on the Azure deployment. In fact, the HTML in the Azure version is different from this branch:

the "Bevestig" button is missing class names from the HolonButton component (in 9c44bf1 I added the ability to pass extra class names to the component)
there a wrapper div (with max-width: 48rem) missing

My guess is that some of this was lost or overwritten when it was merged with your main branch, perhaps due to conflicts with other branches. I'm reasonably confident this won't be an issue, but if it is I will look into it again.

mattijsstam · 2022-07-05T14:23:59Z

frontend/components/FeedbackBlock/SubscriptionForm.js

+      body: JSON.stringify(payload),
+    })
+      .then(() => dispatch({ type: "resolve" }))
+      .catch(() => dispatch({ type: "reject" }));


@antw Within Witteveen+Bos we always use axios because it has a more user friendly api and prevents this sort of (to me) illogical things.

mattijsstam · 2022-07-07T09:54:07Z

Looks good to me. I think this can be merged. 👍

I tested adding a couple of registrations and ratings, and everything worked as I expected.

@mattijsstam I cannot reproduce the styling issues seen on the Azure deployment. In fact, the HTML in the Azure version is different from this branch:

the "Bevestig" button is missing class names from the HolonButton component (in 9c44bf1 I added the ability to pass extra class names to the component)

there a wrapper div (with max-width: 48rem) missing

My guess is that some of this was lost or overwritten when it was merged with your main branch, perhaps due to conflicts with other branches. I'm reasonably confident this won't be an issue, but if it is I will look into it again.

@antw Let's merge it and solve issues when they appear in production, can you resolve the merge conflicts and merge it?

…for-updates-on-the-holon-development

broekhuisg added 3 commits July 4, 2022 14:48

Swagger documentation and models setup

79ba81a

Adding basic permissions

b670133

Minor changes, admin and Readme change

bc2c8ce

broekhuisg requested a review from mattijsstam July 5, 2022 06:32

broekhuisg self-assigned this Jul 5, 2022

broekhuisg linked an issue Jul 5, 2022 that may be closed by this pull request

Users can register for updates on the HOLON development #7

Closed

4 tasks

Update update_register.py

d050124

Door niet de hele modelviewset vanuit rest framework in te laden maar alleen de GenericViewSet en de CreateModelMixin kun je afvangen dat alleen een post mogelijk is zonder zelf permissions te moeten schrijven.

mattijsstam requested changes Jul 5, 2022

View reviewed changes

Only show Swagger when DEBUG is True

3e53d54

antw force-pushed the 7-users-can-register-for-updates-on-the-holon-development branch 2 times, most recently from 36cb4a1 to f08ce87 Compare July 5, 2022 10:41

antw added 2 commits July 5, 2022 11:53

Frontend adjustments for Docker Compose

ba9fa69

- Uses yarn to run the app in development mode. - node_modules are stored in a volume.

antw force-pushed the 7-users-can-register-for-updates-on-the-holon-development branch from f08ce87 to ba9fa69 Compare July 5, 2022 10:56

antw mentioned this pull request Jul 5, 2022

Add a production Dockerfile for the front end #14

Merged

broekhuisg added 3 commits July 5, 2022 14:04

Seperated rating from Register, with new endpoint

27decdd

Merge branch '7-users-can-register-for-updates-on-the-holon-developme…

ad5d272

…nt' of github.com:ZEnMo/Holon-webapp into 7-users-can-register-for-updates-on-the-holon-development

Merged...

9a59038

broekhuisg requested a review from mattijsstam July 5, 2022 12:30

broekhuisg added 2 commits July 5, 2022 14:45

Fixing the connection frontend > backend

1cdadc8

Changed Yarn to NPM

4ae7e8a

mattijsstam requested changes Jul 5, 2022

View reviewed changes

antw added 2 commits July 5, 2022 20:52

Show server validation errors on feedback form

61e2d83

Fix Sentiment test which was broken in 1cdadc8

29517cc

broekhuisg added 2 commits July 6, 2022 13:20

PUT request when there is a choice already made

9db0d2a

Linting...

4442615

broekhuisg requested review from mattijsstam and antw July 6, 2022 11:51

thesethtruth linked an issue Jul 6, 2022 that may be closed by this pull request

Users can update their sentiment after submission by clicking a different sentiment option #15

Closed

await when parsing registration JSON

2fa25f7

Fixes that the errors object would be set to a promise rather than the errors JSON.

antw approved these changes Jul 7, 2022

View reviewed changes

Fix missing dependency list in Sentiment useEffect

e9fc17d

mattijsstam approved these changes Jul 7, 2022

View reviewed changes

antw added 2 commits July 7, 2022 11:23

Merge remote-tracking branch 'origin/main' into 7-users-can-register-…

8b28430

…for-updates-on-the-holon-development

Add validation for HolonButton tag prop

aea2043

antw merged commit ad135b3 into main Jul 7, 2022

antw deleted the 7-users-can-register-for-updates-on-the-holon-development branch July 7, 2022 10:25

patrickkerling mentioned this pull request May 29, 2024

Opzet van pagina's praktijkvoorbeelden aanpassen #973

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7 users can register for updates on the holon development #13

7 users can register for updates on the holon development #13

broekhuisg commented Jul 5, 2022

mattijsstam left a comment

mattijsstam Jul 5, 2022

mattijsstam Jul 5, 2022

mattijsstam Jul 5, 2022

mattijsstam Jul 5, 2022

antw commented Jul 5, 2022

mattijsstam left a comment

mattijsstam Jul 5, 2022

mattijsstam Jul 5, 2022

antw Jul 5, 2022

antw Jul 6, 2022

broekhuisg Jul 6, 2022

thesethtruth commented Jul 5, 2022

mattijsstam commented Jul 6, 2022

broekhuisg commented Jul 6, 2022

broekhuisg commented Jul 6, 2022 •

edited

Loading

thesethtruth commented Jul 6, 2022

antw left a comment

mattijsstam Jul 5, 2022

mattijsstam commented Jul 7, 2022

7 users can register for updates on the holon development #13

7 users can register for updates on the holon development #13

Conversation

broekhuisg commented Jul 5, 2022

mattijsstam 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

antw commented Jul 5, 2022

mattijsstam 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

thesethtruth commented Jul 5, 2022

mattijsstam commented Jul 6, 2022

broekhuisg commented Jul 6, 2022

broekhuisg commented Jul 6, 2022 • edited Loading

thesethtruth commented Jul 6, 2022

antw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattijsstam commented Jul 7, 2022

broekhuisg commented Jul 6, 2022 •

edited

Loading