Research Module - create/edit #1116

danitrod · 2021-03-19T01:32:54Z

PR Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

PR Checklist

- Latest master branch merged
- PR title descriptive (can be used in release notes)
- Passes Tests

Description

Adding create/edit research forms, and create research update form. A couple of notes:

when accessing the URL to directly edit an existing research or add an update to it (/research/<slug>/edit or /research/<slug>/create-update), the store takes a while to validate if a user is logged in or not. I found no way for now to assure the user store has tried loading the user, so I added a forced timeout of 3 seconds to load the user before the code makes any decisions (visible in lines 21-26 of CreateUpdate). Its probably best if we can take the "triedToLogin" information from the user store directly.
I implemented moderation for the researches but couldn't test accepting/rejecting a research as I do not have an admin account.

Git Issues

Closes #1111

Screenshots/Videos

cypress · 2021-03-19T01:42:15Z

Test summary

62 • 0 • 0 • 0

Run details


Project	onearmy-community-platform
Status	Passed
Commit	`f603f0d` ℹ️
Started	Mar 21, 2021 10:58 PM
Ended	Mar 21, 2021 11:04 PM
Duration	05:32 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Multiple

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

github-actions · 2021-03-19T11:55:11Z

Visit the preview URL for this PR (updated for commit 292b6c4):

https://onearmy-next--pr1116-research-form-y2c0ejg8.web.app

_{(expires Tue, 20 Apr 2021 23:02:55 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

davehakkens · 2021-03-19T20:32:53Z

wauw again a super nice pull request! 💪

Not a coder myself, so can't comment to much on code, i'll leave that to @chrismclarke.
Few remarks from testing it.

Image upload doesn't work here.
I get a message when saving: "you have unsaved changes, are you sure to leave the page" (but it looks like you get this as well)
Notifications is not a must in the beginning. We would start using the module internally first for a while with a selected group of people. No need for verification or moderation.
I started a research and added a few steps. When I changed the title of my research all the steps dissapeared
In how-to's we have the original step + all the following steps in one page. (so you can edit all of the steps in one place and also move certain steps up or down) Looks like you splitted this up, each update having its own "page". Any specific reason for this? And how could you edit an update?

danitrod · 2021-03-19T21:43:06Z

Thank you for testing it and for the valuable feedback!

not reallly sure why that happens, but trying in dev mode it works ok. I think its probably something related to preview mode, as I just tried to create a new how-to with images in the preview and the images don't load as well. Maybe @chrismclarke has some insight?
yeah, that's kinda weird, ill try to understand what's happening and fix it
cool, no worries then, ill just make any new research be published as 'accepted' for now
definitely a bug, forgot about the updates on editing a research - will fix
I was thinking the UI for creating a research should be split for the initial intro and each following update, as the updates will be normally created one by one - i might have misunderstood, is it supposed to work like this? About editing an update, its not implemented for now but i can surely work on it, the only question is if they should be edited separately or edited like a howto, all in a single page.

I'll take a look at those bugs and commit fixes soon.

davehakkens · 2021-03-19T21:55:50Z

Tested on dev now, works indeed. Just not the preview link. not a big deal.
Yeh I'm up for trying it the way you made it (separated). Seems more clear and perhaps you don't want to move steps around anyway since it's all chronological time-based anyway. Would be good to add that "edit update" though.

davehakkens · 2021-03-22T19:03:39Z

SUPER neat. it passes all my tests 💪

chrismclarke · 2021-04-05T10:56:49Z

src/components/EventCard/EventCard.tsx

@@ -32,7 +32,7 @@ export const EventCard = (props: IProps) => (
    data-eventId={props.event._id}
  >
    {props.event.moderation !== 'accepted' && (
-      <ModerationStatusText event={props.event} top={'0px'} />
+      <ModerationStatusText moderable={props.event} kind="event" top={'0px'} />


Good to see this refactored into a common component, I don't really understand what the word moderable means, but I can see you inherited the code from others so definitely not blaming!

Would you have any suggestion for a word that makes more sense? maybe moderatedContent or similar? Similarly we could change the word kind to something a bit more specific (e.g. contentType) to make things clearer for future devs.

But these are minor issues so I might just add a small comment myself to tidy in the future.

Agreed, I was struggling to find a good name for this but definitely moderatedContent and contentType would be more describing names. Should I open a PR for this?

chrismclarke · 2021-04-05T11:09:30Z

src/pages/Research/Content/Common/Research.form.tsx

+
+  React.useEffect(() => {
+    if (submissionHandler.shouldSubmit) {
+      const form = document.getElementById('researchForm')


I think react probably prefers when people use refs (https://reactjs.org/docs/refs-and-the-dom.html) instead of accessing dom directly, but I'm not aware of any issues doing it the vanilla way (the id is defined well enough to avoid multiple elements with the same id).
A bit more here: https://stackoverflow.com/questions/37273876/reactjs-this-refs-vs-document-getelementbyid

I think I was adapting some of the logic from the HowTo form, which used this approach. Definitely a future improvement to not access the DOM directly.

chrismclarke · 2021-04-05T11:17:00Z

src/pages/Research/Content/EditUpdate/index.tsx

+          setTimeout(() => {
+            loggedInUser = store.activeUser
+            resolve()
+          }, 3000),


I think this is something you mentioned in your comments - agreed it's a pain to know whether there is no user or if they just haven't finished login initialisation after direct nav / page reload. This workaround is fine for now, I think there would probably be some better way long term to handle more generally but I'll create a separate issue for that (#1124)

chrismclarke

There's a huge amount here, which is really appreciated but also a bit tricky to review in depth. Overall the code looks really solid (I've added a few minor comments), so happy to merge. I think we can do some manual testing to check if everything is behaving as expected, and just make minor issues to fix anything as we come across it.

As the majority of the code is nicely encapsulated within the research module we can test most from that page, but for reference @davehakkens - there's also a few updates included to re-organise/re-use some of the content from howtos and events (specifically moderation status) - so will be good to double check that is still working as expected too.

davehakkens · 2021-04-05T16:51:47Z

Got this message in Slack today. Are they related?

danitrod added 6 commits March 15, 2021 00:30

add 'add research' form

0fe965a

add moderation view to research

47f16c4

setup research moderation, drafting and filtering

864e26e

setup research editing

97e5251

setup update adding

9f49f09

research module creating: fixes and tidying

81f6ccd

github-actions bot added Mod: HowTo 📰 Mod: Research 🔬 labels Mar 19, 2021

davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Mar 19, 2021

pr ONEARMY#1116 dave review

292b6c4

davehakkens mentioned this pull request Mar 22, 2021

Research module live, but only for specific users #1118

Closed

chrismclarke self-requested a review April 5, 2021 10:45

chrismclarke reviewed Apr 5, 2021

View reviewed changes

chrismclarke approved these changes Apr 5, 2021

View reviewed changes

chrismclarke merged commit ee423fe into ONEARMY:master Apr 5, 2021

danitrod deleted the research-form branch April 5, 2021 12:40

danitrod mentioned this pull request Apr 5, 2021

Add DB metadata to research updates #1114

Closed

danitrod mentioned this pull request Apr 16, 2021

Research module live, but only for specific users #1129

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Research Module - create/edit #1116

Research Module - create/edit #1116

danitrod commented Mar 19, 2021 •

edited

Loading

cypress bot commented Mar 19, 2021 •

edited

Loading

github-actions bot commented Mar 19, 2021 •

edited

Loading

davehakkens commented Mar 19, 2021

danitrod commented Mar 19, 2021 •

edited

Loading

davehakkens commented Mar 19, 2021 •

edited

Loading

davehakkens commented Mar 22, 2021

chrismclarke Apr 5, 2021

danitrod Apr 5, 2021

chrismclarke Apr 5, 2021 •

edited

Loading

danitrod Apr 5, 2021

chrismclarke Apr 5, 2021 •

edited

Loading

chrismclarke left a comment

davehakkens commented Apr 5, 2021

Research Module - create/edit #1116

Research Module - create/edit #1116

Conversation

danitrod commented Mar 19, 2021 • edited Loading

Description

Git Issues

Screenshots/Videos

cypress bot commented Mar 19, 2021 • edited Loading

Test summary

Run details

github-actions bot commented Mar 19, 2021 • edited Loading

davehakkens commented Mar 19, 2021

danitrod commented Mar 19, 2021 • edited Loading

davehakkens commented Mar 19, 2021 • edited Loading

davehakkens commented Mar 22, 2021

chrismclarke Apr 5, 2021

Choose a reason for hiding this comment

danitrod Apr 5, 2021

Choose a reason for hiding this comment

chrismclarke Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

danitrod Apr 5, 2021

Choose a reason for hiding this comment

chrismclarke Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

chrismclarke left a comment

Choose a reason for hiding this comment

davehakkens commented Apr 5, 2021

danitrod commented Mar 19, 2021 •

edited

Loading

cypress bot commented Mar 19, 2021 •

edited

Loading

github-actions bot commented Mar 19, 2021 •

edited

Loading

danitrod commented Mar 19, 2021 •

edited

Loading

davehakkens commented Mar 19, 2021 •

edited

Loading

chrismclarke Apr 5, 2021 •

edited

Loading

chrismclarke Apr 5, 2021 •

edited

Loading