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(videos): integration fixes for video appointment upload flow #393

Closed
wants to merge 15 commits into from

Conversation

connordoman
Copy link
Contributor

@connordoman connordoman commented Jan 25, 2024

Welcome to PrivacyPal! 👋

Fixes: #357
Depends on #473

Description of the change:

Naturally, this PR grew to be annoyingly large.

This PR addresses many small to medium aspects of the user path for uploading a video to an appointment.

Some stand out changes are:

  • dummy-authenticator now checks the database for credentials (aligns db and basic auth. db provider would be a more robust solution)
  • Adds hashed passwords in user.properties.json in order for said auth to work
  • appointment list in dashboard features links to appointments
  • basic <AppointmentViewer /> component that acts as a stand-in for the real feature
  • the viewer links to /upload?id=${appointmentId} and will raise 404 if the user is not authorized and/or the wrong or no id is specified
  • /upload/status uses this id as well (+ request filtering)
  • /upload/review uses this id as well (+ request filtering)
  • All filtering happens server-side with RSC + actions.ts
  • Failed uploads at /review return the user to the appropriate /upload page
  • Successful uploads return the user to the relevant appointment (where the video would be viewable in a list)
  • Server Action to query users by email
  • Server Action to query users without returning their password
  • Server Action to get a professional's appointment
  • Server Action to get a client's appointment
  • Server Action to check if a user is part of a given appointment
  • probably some more stuff

Sorry for such a large PR. Some of these could have been on their own.

Also, smoketest is outdated, so thoroughly testing this feature set was challenging. I suspect it will work but thoroughly investigating is not worth it until new video processing has been implemented.

Motivation for the change:

Integration and cohesiveness

@connordoman connordoman added feat New feature or request fix Fixes for something broken area/front-end Front-end work area/back-end Back-end work labels Jan 25, 2024
@connordoman connordoman added this to the Term 2 Week 4 ❗️ milestone Jan 25, 2024
@connordoman connordoman self-assigned this Jan 25, 2024
@tthvo tthvo changed the title feat(privacy-pal): integration fixes for video appointment upload flow feat(videos): integration fixes for video appointment upload flow Jan 25, 2024
@tthvo tthvo added the high-priority Need immediate attention label Jan 26, 2024
@tthvo
Copy link
Contributor

tthvo commented Jan 26, 2024

@nganphan123 @linhnnk ping.

@connordoman
Copy link
Contributor Author

@nganphan123 @linhnnk piiiing

Copy link
Contributor

@nganphan123 nganphan123 left a comment

Choose a reason for hiding this comment

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

There will be conflicts with this PR #406 when either is merged. We should consider which one to merge first to minimize the conflict fix.

app/web/db/schema.prisma Outdated Show resolved Hide resolved
app/web/src/app/actions.ts Outdated Show resolved Hide resolved
app/web/src/app/actions.ts Outdated Show resolved Hide resolved
app/web/src/app/actions.ts Outdated Show resolved Hide resolved
app/web/src/app/staff/appointment/page.tsx Show resolved Hide resolved
@nganphan123
Copy link
Contributor

I tested the app, everything works for me. Nice work!
I tried uploading video but it said no appointment id found. Does this PR include uploading video flow?
Screenshot 2024-01-27 223612

@connordoman
Copy link
Contributor Author

I tested the app, everything works for me. Nice work! I tried uploading video but it said no appointment id found. Does this PR include uploading video flow? Screenshot 2024-01-27 223612

The flow that works is

  1. pro creates appointment
  2. user finds appointment on dashboard
  3. user clicks "upload video" from appointment page
  4. this passes relevant data to /upload
  5. flow continues with data being passed across routes until processed/declined

@MyStackOverflows
Copy link
Contributor

Yeah took me a bit because I didn't realize I had to re-migrate the database (duh) but after testing I get to the same point as Ngan, video upload says "no appointment id provided"

@MyStackOverflows
Copy link
Contributor

image
trying to build these changes and getting this, did the db schema/column names change?

@connordoman
Copy link
Contributor Author

image trying to build these changes and getting this, did the db schema/column names change?

It did. Fixed this but can't continue till Basic auth is merged

@MyStackOverflows
Copy link
Contributor

It did. Fixed this but can't continue till Basic auth is merged

could that db change be why my attempts at creating appointments on #426 are failing? if so (since the actual auth part of the PR works) we can merge it and then work on this @connordoman

@connordoman connordoman requested review from tthvo, linhnnk and nganphan123 and removed request for tthvo February 5, 2024 03:49
@github-actions github-actions bot added dependent Depending on other work and removed dependent Depending on other work labels Feb 5, 2024
Copy link

github-actions bot commented Feb 7, 2024

This PR/issue depends on:

@tthvo
Copy link
Contributor

tthvo commented Feb 7, 2024

Hey team @COSC-499-W2023/team-1, @connordoman and I had a discussion on this and we agreed that it would better to split this PR into smaller ones.

Please prioritize reviewing #402 and #395 for wrap up appointment-related tasks.

@tthvo tthvo deleted the gh-357-reintegrate-video-upload branch April 6, 2024 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/back-end Back-end work area/front-end Front-end work feat New feature or request fix Fixes for something broken high-priority Need immediate attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants