-
Notifications
You must be signed in to change notification settings - Fork 34
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
On-Demand Report backend #226
base: main
Are you sure you want to change the base?
Conversation
The commit 146957c works to retrieve all of the checked-out items but will require updating the test database. It's likely that we will need to run a total count similar to summary.js/within the backend calls in order to retrieve total numbers and counts. |
The current plan is to open another PR that will handle the frontend changes necessary to link the Summary.js with the backend. There we will be able to calculate the total number of checkouts and unique students necessary to fulfill task 3 and 4. |
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.
@jayrevolinskyjr This looks good but could you add authentication to the 3 endpoints ? I see that ensureAuthenticated has been passed as a parameter but you'd have to add an if-else statement to ensure authentication. Something like if (!req.isAuthenticated()) { res.status(401).send("Unauthenticated"); return; } else{ //rest of your code }
You can lookup donor.js file for reference.
@parthpandey1 I have addressed the changes by adding if-else authentication checks on all of the /items routes. The screenshot shows an example of the routes behaving properly when an authenticated or unauthenticated request is made on a given route. |
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.
Looks good! I went ahead and tested the endpoints using postman with authentication and it works flawlessly
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.
/total_donation and /total_checkouts have been tested and look good. Authentication messaging edits also tested and looks good.
For /unique_checkouts, we'll need richer dummy data with multiple person_ids to ensure this is working properly (currently only a single person_id is being used for INSERTs)
@jayrevolinskyjr and I attempted to create additional inserts with different person_ids but with no luck. Will require further prodding.
@reembot I updated the dummy_data with an additional person and added an explicit checkout option that was missing. @parthpandey1 It appears that the Feed tests are failing since they rely on dummy data. I will investigate further for fixing the Feed tests related to Issue #208. It's likely Feed.test will need to be updated before any more merges can be done. Update 05/13/2023: Feed tests were fixed with #243, rebased with main to pass tests and coverage. |
This is the PR related to Issue #223. Current plan is to implement backend before the frontend is designed/started.