DEP-277: Add CHEFS + Object storage file upload functionality to DEP (+fix survey listing) - #2892
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2892 +/- ##
==========================================
- Coverage 73.62% 73.30% -0.32%
==========================================
Files 521 523 +2
Lines 20937 21092 +155
Branches 1844 1841 -3
==========================================
+ Hits 15415 15462 +47
- Misses 5512 5620 +108
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jareth-whitney
left a comment
jareth-whitney
left a comment
There was a problem hiding this comment.
Great work on this CHEFS, object storage, and general form cleanup. I figure if we are good stewards of the app then these little improvements come naturally. Nothing blocking, just check out the following:
- authorization message specificity
- Number.isNaN vs Number.isFinite or similar
- Checking that spread of two objects is shallow
- Console.log (possibly intentional)
- Token validation vs simple existence check
- surveyId Number conversion, consider converting once
| f'membership.tenant_id:{membership.tenant_id} ' | ||
| f'user_from_context.tenant_id: {g.tenant_id}') | ||
| abort(HTTPStatus.FORBIDDEN, UNAUTHORIZED_MSG) | ||
| flask_abort(HTTPStatus.FORBIDDEN, UNAUTHORIZED_MSG) |
There was a problem hiding this comment.
Thank you for giving the authorization service a refresh.
| if not self.is_configured(): | ||
| return { | ||
| 'status': 'Configuration Issue', | ||
| 'message': 'accesskey is None or secretkey is None or S3 host is None or formsbucket is None' |
There was a problem hiding this comment.
Just wondering if this error is for the log or returning to the user. We could optionally give users a more generic message.
| @classmethod | ||
| def get(cls, survey_id): | ||
| """Get survey by the ID.""" | ||
| """Get a survey by its ID.""" |
There was a problem hiding this comment.
Love these little grammar fixes, very much my style.
| try { | ||
| if (isNaN(Number(submissionId))) { | ||
| throw new Error(); | ||
| if (Number.isNaN(Number(submissionId))) { |
There was a problem hiding this comment.
Just checking if you're specifically looking for NaN or want to check if it's a valid number. You could use something like Number.isFinite() and >= 0 for example.
| import { Link } from 'components/common/Navigation'; | ||
|
|
||
| const FailurePanel = ({ email, handleClose, tryAgain, isInternal }: FailurePanelProps) => { | ||
| /* TODO: Populate this with the tenant configuration from the API */ |
There was a problem hiding this comment.
Love the TODO's, please ignore SonarCloud!
| return fetchSurveyReportSettings(String(s.id)); | ||
| }); | ||
| const submission = verification?.then((verif) => { | ||
| if (!verif?.verification_token) return null; |
There was a problem hiding this comment.
Do we need to validate the token or is its existence enough?
There was a problem hiding this comment.
If the server response contains a verification_token field, it means that a valid verification resource was found
| const { survey } = useSurveyLoaderData(); | ||
| const isLoggedIn = useAppSelector((state) => state.user.authentication.authenticated); | ||
| const surveyLoaderData = useSurveyLoaderData(); | ||
| const isManagementPath = useIsManagementRoute(); |
There was a problem hiding this comment.
I love all of these custom little hooks, very cool.
| <Suspense | ||
| fallback={ | ||
| <Button disabled> | ||
| {translate('surveySubmit.surveyForm.button.cancel')} |
There was a problem hiding this comment.
Thank you for adding translation strings here.
| // we can still expect the survey data from the nearest route's loader | ||
| const fallbackData = useLoaderData(); | ||
|
|
||
| return (surveyLoaderData ?? publicSurveyLoaderData ?? fallbackData) as SurveyLoaderData; |
There was a problem hiding this comment.
Do we want to fire all these loaders at once? I guess it's a trade off, performance vs resource conservation. You could request loader data 2 if loader data 1 doesn't arrive, for example.
There was a problem hiding this comment.
But they're not firing! That's why loaders are so efficient - you can't start them more than once. The loaders are triggered by the user's navigation to the page, and copies of the promise(s) are made available to every call of useLoaderData(). The routes are set up so that no 2 survey loaders can be active at once. Thus, calling useLoaderData() on a route where the loader is not triggered returns undefined and results in 0 XHRs
| - Added a new public document endpoint to the API that generates pre-signed URLs for uploading files to Object Storage. This endpoint takes the file name and content type as input and returns a pre-signed URL that can be used to upload the file directly to Object Storage. | ||
| - The new API can also be used to generate pre-signed URLs for downloading files from Object Storage, as well as deleting files from Object Storage. | ||
| - Updated the Formio options to include callbacks for handling file uploads, downloads, and deletions. These callbacks use the new API to generate URLs and perform privileged operations on Object Storage, allowing users to upload files during survey submission without needing high-level access to the storage service. | ||
| - Cleaned up survey-related component code |
There was a problem hiding this comment.
Thank you for implementing the Midas touch here!
|
3 participants
Issue #: 🎟️ DEP-277
Description of changes:
User Guide update ticket (if applicable):
Common component changes: