-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
@nganphan123 @linhnnk ping. |
@nganphan123 @linhnnk piiiing |
There was a problem hiding this 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.
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" |
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 |
This PR/issue depends on:
|
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. |
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)user.properties.json
in order for said auth to work<AppointmentViewer />
component that acts as a stand-in for the real feature/upload?id=${appointmentId}
and will raise 404 if the user is not authorized and/or the wrong or noid
is specified/upload/status
uses this id as well (+ request filtering)/upload/review
uses this id as well (+ request filtering)actions.ts
/review
return the user to the appropriate/upload
pageSorry 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