Conversation
navyaa31
left a comment
There was a problem hiding this comment.
When I upload the bad_phone and bad_email csvs, I get this error. However, this isn't there for the frontend only code so do you know what might be causing this?
Also, could you resolve merge conflicts with main? Since there have been some changes to the schema, the tags are rendering incorrectly on this branch so we should make sure that doesn't happen before merging this PR
| ); | ||
|
|
||
| if (!result.ok) { | ||
| alert("Failed to upload volunteers: " + result.error); |
There was a problem hiding this comment.
We can delete the alerts!
Not sure if I fully understand the question, but if you're asking where the CSV headers being validated, everything should be in the backend. The one bit in the frontend to check the file extension was redundant and I just removed it. Also I merged main back in and updated the api to use the new backend auth as well as update the csv parser to use the new tag backend. However, I wasn't able to reproduce the tag display error. I don't think I touched that section of the frontend, but maybe some of the other changes fixed it. Let me know if that error persists. Also I'm curious how you're seeding the volunteers. If there's a new script that's compliant with the new volunteer and tag backend, let me know and I can do some more thorough testing. |
siwenshao
left a comment
There was a problem hiding this comment.
I think this PR still includes the full backend CSV implementation in #20 PR which was merged already, and it did not reflect the most up-to-date changes that #20 made. Please split/rebase to avoid duplicating the earlier backend PR.
Overall, it looks great! I left a couple of comments about the details that we might want to look into/fix if needed.
backend/src/middleware/auth.ts
Outdated
| const token = authHeader.split("Bearer ")[1]; | ||
| const token = authHeader.split("Bearer ")[1]; |
There was a problem hiding this comment.
nit: why do we have a duplicated const token here of the same thing? Maybe remove one.
| storage, | ||
| limits: { fileSize: 1024 * 1024 * 5 }, // 5MB limit | ||
| fileFilter: (req, file, cb) => { | ||
| if (file.mimetype !== "text/csv" || path.extname(file.originalname).toLowerCase() !== ".csv") { |
There was a problem hiding this comment.
As said in the review, I think this part is updated/fixed in the #20 PR but not reflected here
| tags: string[]; | ||
| }; |
There was a problem hiding this comment.
nit: this is not matching what is in frontend/src/app/api/volunteer.ts:tags?: string[]
9a4e7e0 to
138689d
Compare
There was a problem hiding this comment.
Could we have the frontend call the backend endpoints directly and remove these proxy routes? This would make the code easier to maintain. We're going to do this in the rest of the codebase as well soon
There was a problem hiding this comment.
The csv only contained data about new/returning, which I thought that was no longer stored as a tag. If we should still have the volunteer status display as a tag I can do that.
There was a problem hiding this comment.
ohh you are right about the new/returning, ignore my comment about that, thanks for the reminder
siwenshao
left a comment
There was a problem hiding this comment.
Thanks for all the fixes! It looks good to me!

Tracking Info
Relates to #12
Resolves #18
Changes
Merged the CSV processing backend with the frontend display.
Testing
Tested with the following CSV files.
one.csvmany.csvbad_formatting.csvbad_phone.csvbad_email.csvConfirmation of Change
Tested with
one.csvandmany.csvcsv-merge-fullTest.webm
Tested fail cases with
bad_formatting.csv,bad_phone.csv, andbad_email.csvcsv-merge-failTest.webm
Possible Improvements (either future or now, let me know)
string[] tagmember of a volunteer in the database. It should likely be stored as a boolean to prevent volunteers from being both "new" and "returning", but that goes out of the scope of this branch.