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

On-Demand Report backend #226

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

On-Demand Report backend #226

wants to merge 11 commits into from

Conversation

jayrevolinskyjr
Copy link
Contributor

This is the PR related to Issue #223. Current plan is to implement backend before the frontend is designed/started.

@jayrevolinskyjr jayrevolinskyjr linked an issue May 2, 2023 that may be closed by this pull request
4 tasks
@jayrevolinskyjr jayrevolinskyjr added feature New feature or request Urgent Need everyone's attention labels May 2, 2023
@jayrevolinskyjr
Copy link
Contributor Author

jayrevolinskyjr commented May 3, 2023

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.

@jayrevolinskyjr jayrevolinskyjr marked this pull request as ready for review May 5, 2023 22:14
@jayrevolinskyjr
Copy link
Contributor Author

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.

@parthpandey1 parthpandey1 self-requested a review May 8, 2023 17:32
Copy link
Contributor

@parthpandey1 parthpandey1 left a 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.

@jayrevolinskyjr
Copy link
Contributor Author

@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.
UnauthenticatedRoutes

parthpandey1
parthpandey1 previously approved these changes May 8, 2023
Copy link
Contributor

@parthpandey1 parthpandey1 left a 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

Copy link
Contributor

@reembot reembot left a 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.

@jayrevolinskyjr
Copy link
Contributor Author

jayrevolinskyjr commented May 12, 2023

/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.

@jayrevolinskyjr jayrevolinskyjr requested a review from reembot May 12, 2023 00:08
@jayrevolinskyjr jayrevolinskyjr dismissed reembot’s stale review May 12, 2023 00:09

Updated dummy_data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request Urgent Need everyone's attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On-demand report generation
3 participants