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

Add file upload api #267

Merged
merged 75 commits into from
Jan 27, 2021
Merged

Add file upload api #267

merged 75 commits into from
Jan 27, 2021

Conversation

dcmcand
Copy link
Contributor

@dcmcand dcmcand commented Jan 26, 2021

Description of change
This PR adds the a API endpoint POST /files. This is only for the api, and does not include the front end.

How to test

  1. Update .env file with new variables:
    1. AWS_SECRET_ACCESS_KEY=EXAMPLEKEY
    2. AWS_ACCESS_KEY_ID=EXAMPLEID
    3. MINIO_ROOT_PASSWORD=EXAMPLKEY
    4. MINIO_ROOT_USER=EXAMPLEID
    5. S3_BUCKET=ttadp-test
    6. If you aren't using docker, S3_ENDPOINT=http://localhost:9000, or if you are S3_ENDPOINT=http://minio:9000
  2. Start app using yarn docker:start
  3. Go to localhost:9000 to access the Minio web console (use AWS_SECRET_ACCESS_KEY for password and AWS_ACCESS_KEY_ID for username).
  4. if you aren't using docker create a bucket named ttadp-test (It should be created automatically if you are using docker)
  5. Open the web ui at http://localhost:3000 and save a Activity Report. Note the reportId to use in your curl command.
  6. Open a terminal and type curl localhost:3000/api/files -F [email protected] -F reportId=1 -F attachmentType=ATTACHMENT where TESTFILE.txt is a file in the directory you run the curl command from
  7. Output should be similar to
{
"id":5
}
  1. Check the DB to ensure the appropriate metadata was added to the Files table

Issue(s)

Checklist

  • Meets issue criteria
  • Code tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • Documentation updated

@rahearn rahearn self-requested a review January 26, 2021 15:39
}
return new Promise((resolve) => resolve(data));
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should enable versioning on the bucket if it isn't enabled closing #130

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 🎉

Copy link
Contributor

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

Great work! I'd like to update the File name to have the same casing structure as all other API fields, and I had some questions about a couple other things.

reportId:
type: number
description: "id of the Activity report the file is associated with"
File:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for consistency, this should be named file

$ref: '../index.yaml#/components/schemas/fileUpload'
responses:
200:
description: One user by an id
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this description means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is cause it is nonsense. I think it got confused in a merge.

}
return new Promise((resolve) => resolve(data));
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 🎉

Key: name,
};
// Only check for versioning if not using Minio
if (process.env.LOCAL_DEV !== 'true') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop LOCAL_DEV in favor of process.env.NODE_ENV as it's being used here:

if (process.env.NODE_ENV === 'production') {
app.use('*', (req, res) => {
res.sendFile(path.join(__dirname, 'client', 'index.html'));
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good approach.

metadata = await createFileMetaData(originalFilename, fileName, reportId, attachmentType[0]);
} catch (err) {
await handleErrors(req, res, err, logContext);
res.status(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

handleErrors already sets the status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, that simplifies a bit.

});
});
it('checks the metadata was uploaded to the database', async () => {
const file = await File.findAll({ where: { id: fileId } });
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be File.findOne and then all of the file[0] could be replaced with file

};
let file;
try {
await sequelize.transaction(async (transaction) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of wrapping this (and the update call in updateStatus) in a transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I am new to sequalize and found that was how josh had done it elsewhere, so I copied it. I THINK it would be fine not too.

export const s3 = new S3({
accessKeyId: process.env.AWS_ACCESS_KEY_ID,
endpoint: process.env.S3_ENDPOINT,
secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have another requested change for this as well. In deployed environments, we should pull these variables (as well as the bucket name) out of the process.env.VCAP_SERVICES environment variable.

Copy link
Contributor

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

:shipit:

@rahearn rahearn merged commit 790ca05 into HHS:main Jan 27, 2021
dcmcand referenced this pull request in adhocteam/Head-Start-TTADP Feb 10, 2021
commit d1c5786
Merge: 44eeeb8 059425c
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 17:10:20 2021 -0500

    Merge pull request #157 from HHS/user-last-login

    Allow filtering users by last login and access permissions

commit 059425c
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 15:25:09 2021 -0500

    Make eslint happy

commit 09e906f
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 14:46:50 2021 -0500

    Refactor user filtering to match access control SOP rules directly

commit facfee4
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 14:03:29 2021 -0500

    Allow filtering by only showing users who do have SITE_ACCESS

commit b5b93f0
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 13:40:44 2021 -0500

    Allow filtering by last login > 60 or 180 days ago

    Options to match process here: https://github.com/HHS/Head-Start-TTADP/wiki/Access-Control-&-Account-Management-SOP#account-review-frequency-and-process

commit 1d08788
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 12:18:39 2021 -0500

    Display lastLogin on admin user details

commit 31883dd
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 11:12:00 2021 -0500

    Add lastLogin to user api response

commit 60bbefd
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 10:20:03 2021 -0500

    Add lastLogin value to user model & update on login

commit 1a2dd2c
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 09:36:09 2021 -0500

    Prevent validation issues if HSES email updates

commit 7554928
Merge: 875f5e2 44eeeb8
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 9 14:15:51 2021 -0500

    Merge pull request #286 from adhocteam/main

    Fix cucumber test on CI

commit 875f5e2
Merge: 4bd8566 9f5bb95
Author: Ryan Ahearn <[email protected]>
Date:   Fri Feb 5 16:31:48 2021 -0500

    Merge pull request #284 from adhocteam/main

    Filter locked users from admin list

commit 4bd8566
Merge: 17f9d7f f327246
Author: Ryan Ahearn <[email protected]>
Date:   Fri Feb 5 16:05:29 2021 -0500

    Merge pull request #281 from adhocteam/main

    Case insensitive admin search, testing updates and manager setting of notes and report status

commit 17f9d7f
Merge: 16d54e2 8e5d3ef
Author: Ryan Ahearn <[email protected]>
Date:   Wed Feb 3 16:09:21 2021 -0500

    Merge pull request #279 from adhocteam/main

    add frontend for file upload

commit 16d54e2
Merge: 541640a e27e7df
Author: Ryan Ahearn <[email protected]>
Date:   Tue Feb 2 15:33:25 2021 -0500

    Merge pull request #275 from adhocteam/main

    Add goal component to frontend

commit 541640a
Merge: 790ca05 4c3a436
Author: Ryan Ahearn <[email protected]>
Date:   Fri Jan 29 15:47:13 2021 -0500

    Merge pull request #272 from adhocteam/main

    Save collaborators to report

commit 790ca05
Merge: 4948bd3 3cff2f4
Author: Ryan Ahearn <[email protected]>
Date:   Wed Jan 27 12:04:04 2021 -0500

    Merge pull request #267 from adhocteam/main

    Add file upload api
rahearn pushed a commit that referenced this pull request May 10, 2021
…ctivity-reports

Add ability to delete a report in My Alerts
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.

Selecting topics from the unfiltered list of all topics As a user, I want my uploaded files backed up (CP-9)
3 participants