EDFIAL-576 Make unmatched-student-file-specific download url endpoint - #116
EDFIAL-576 Make unmatched-student-file-specific download url endpoint#116rtavernaea wants to merge 5 commits into
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
rtavernaea
changed the title
edandylytics
left a comment
edandylytics
left a comment
There was a problem hiding this comment.
Overall, this is very good! Haven't tested yet, but I expect it'll test well. I made a couple small notes, mostly thinking about code we might want to keep as is so we can use it in the next PR for allowing the download of output files
| }) => { | ||
| const url = await apiClient.get<string, string>( | ||
| `/jobs/${jobId}/output-files/${encodeURIComponent(fileName)}` | ||
| `/jobs/${jobId}/output-files/input_no_student_id_match.csv` |
There was a problem hiding this comment.
I think we'll still want this front-end query to take a parameterized file name, though the file name would be hard coded in UnmatchedStudents.tsx. That'll allow us to use this query for the other run output files we'll want to enable for download, too.
|
|
||
| get unmatchedStudentsFile() { | ||
| return this.lastRun?.unmatchedStudentsFile; | ||
| get hasUnmatchedStudents() { |
There was a problem hiding this comment.
I like this! One thing we should check on is when we added the unmatchedStudentsInfo. I think it was long enough ago that backwards compatibility isn't a big concern, but at was added after the app was live in prod.
| } | ||
|
|
||
| @Get(':jobId/output-files/:fileName') | ||
| @Get(':jobId/output-files/input_no_student_id_match.csv') |
There was a problem hiding this comment.
I'd lean towards adding this as a new endpoint, alongside (and above, so it matches first) the parameterized version, as you'd need to re-implement that anyway in the next PR. I don't feel strongly about that.
There was a problem hiding this comment.
Though including it in this PR would allow you to add some tests in a fairly natural spot: test asserting that no special permission is need for accessing the input_no_student_id_match.csv handler, but you do need permissions to access the output files.
There was a problem hiding this comment.
Would that test just cover that 'read:jobs:output-files' is required for @Get(':jobId/output-files/:fileName'), but not required for @Get(':jobId/output-files/input_no_student_id_match.csv')?
Or is there something further I'm missing here?
2 participants
ticket
downloadUrlForOutputFileto:jobId/output-files/input_no_student_id_match.csvand removes the fileName paramunmatchedStudentsFilegetterhasUnmatchedtudentsgetter that reads fromunmatchedStudentsInfofor hiding/displaying the Unmatched Students component on the front end