Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add admin manual CheckInReady toggle #1448

Merged
merged 2 commits into from
Jul 23, 2023

Conversation

trillium
Copy link
Member

@trillium trillium commented Jul 18, 2023

Fixes #1447

What changes did you make and why did you make them ?

  • Create EventsApiService that mimics RecurringApiService
  • Add section that renders events from /events/ that match the project in the projects page
  • Adapt the surrounding components to pass needed props

Screenshots of Proposed Changes Of The Website

Visuals before changes side by side image image image image image image image image image

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b Spiteless-ts.checkInReady_toggle development
git pull https://github.com/Spiteless/VRMS.git ts.checkInReady_toggle

@@ -36,7 +36,7 @@ const EditProject = ({
recurringEvents
// eslint-disable-next-line no-underscore-dangle
.filter((e) => e?.project?._id === projectToEdit._id)
.map((item) => readableEvent(item))
.map((item) => ({...item, ...readableEvent(item)}))
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is allows checkInReady to be passed via props


return (
<li key={`${event.event_id}`}>
<button type="button" onClick={() => {console.log(event);setSelectedEvent(event)}}>
Copy link
Member Author

@trillium trillium Jul 18, 2023

Choose a reason for hiding this comment

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

setSelectedEvent designates which event should be editable in the modal rendered by <EditMeetingTimes /> and passes on all the event values.

@@ -15,6 +15,7 @@ const EditableMeeting = ({
handleEventDelete,
formErrors,
videoConferenceLink = '',
checkInReady,
Copy link
Member Author

Choose a reason for hiding this comment

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

Ensures checkInReady is destructed off of props

onClick={handleEventUpdate(eventId, {checkInReady: !checkInReady})}
>
ToggleCheckIn({!checkInReady ? "Yes" : "No"})
</button>
Copy link
Member Author

Choose a reason for hiding this comment

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

handleEventUpdate() takes an evendId, a values object, and two other props unused here.

The values object is passed an obj with the opposite of checkInReady, and sent to the db updater fuction.

checkInReady: values.checkInReady,
};
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This section updates handleEventUpdate() in the same style as the original function, allowing minimal change for us to update checkInReady in the db.

@MattPereira
Copy link
Contributor

MattPereira commented Jul 19, 2023

@spiteless your changes do successfully update the associated recurringEvent object in the db, however, it does't show up on the check in page for users who are not logged in because the home page component is making a request to the events endpoint. For anyone who is unfamiliar, the recurringEvents are used to create events via the createRecurringEvents.js worker. So there is theoretically an event object in the db for every single time a meeting happens which results in the 16,095 events records that are in the prod db right now, but there are only 67 recurringEvents which generate all those events

I refactored the home page a few months ago and left it hitting api/events because that's how it was before and didn't want to break anything but maybe swapping it out for api/recurringEvents could be an option to explore?

One thing to keep in mind is that if you manually flip it to checkInReady: true you would also have to manually flip it back to checkInReady: false or the meeting would remain as always check in ready. If you swap the endpoint to /api/recurringEvents?checkInReady=true on your local env you will see about 40 recurringEvents sporting a check in ready true coming from the test db

const res = await fetch('/api/events?checkInReady=true', {

@MattPereira
Copy link
Contributor

MattPereira commented Jul 19, 2023

maybe swapping it out for api/recurringEvents could be an option to explore?

This might not be a good idea since the workers only automate the open and close of events so changing the endpoint that gets hit by the homepage to /api/recurringEvents?checkInReady=true would also require changing the worker files that open and close the events to target recurringEvents instead

@@ -207,4 +198,21 @@ const EditProject = ({
);
};

function RecurringEvent({event, setSelectedEvent}) {

Copy link
Member

Choose a reason for hiding this comment

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

Small nit: remove empty line

Copy link
Member

@evanyang1 evanyang1 left a comment

Choose a reason for hiding this comment

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

LGTM! Other perspectives welcome

@MattPereira
Copy link
Contributor

@evanyang1 how did you test that users would be able to check into meetings that are manually flipped?

am i misunderstanding the goal for this PR?

Screenshot from 2023-07-19 17-46-09

{`${event.dayOfTheWeek}, ${event.startTime} - ${event.endTime}; ${event.eventType}`}
<div className="edit-icon"><EditIcon /></div>
</div>
<div className="event-list-description">{`Can uses check into this event now?: ${event.checkInReady ? "Yes" : "No"}`}</div>
Copy link
Member

Choose a reason for hiding this comment

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

check spelling

@trillium
Copy link
Member Author

Updated this commit

Copy link
Contributor

@MattPereira MattPereira left a comment

Choose a reason for hiding this comment

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

@spiteless hey man well done figuring this one out! 🍻

The feature works as expected, but in the process of testing I discovered that duplicate events are sneaking into the test db

See screenshot

Screenshot from 2023-07-23 15-16-53

Luckily, it doesn't appear as though there are duplicate events in the prod db

See screenshot

Screenshot from 2023-07-23 15-29-34

Therefore, I believe your PR will successfully provide PMs with the ability to manually flip the event to checkInReady: true

@trillium trillium merged commit 7c06453 into hackforla:development Jul 23, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Admin to manually toggle if an event is checkInReady
4 participants