1006/1007/1008 created new API endpoints for front end statistics #1059
Conversation
NotificationAllTimeView which is a view created to merge notifiations and notification_history
service, for a user, by a number of days
for REQUESTED attribute
anagradova
changed the title
|
I want to make a note that the only endpoint that's missing is the month without user_id |
There was a problem hiding this comment.
Thank you, @anagradova, this is awesome and really nice progress on getting these endpoints together so quickly!
There are a couple of smaller things I noted in addition to a larger thing I wanted to check on, which is a possible optimization for how the day ranges are retrieved and returned. Please let me know if what I've noted is not clear and/or if you don't think it makes sense to incorporate, I'm definitely happy to sit down and talk through it all!
There is one other thing that I wanted to make sure we took a look at too, and that's using SQLAlchemy's newer (well, current!) style of querying the database: https://docs.sqlalchemy.org/en/20/tutorial/data_select.html
What you have here certainly works and is correct when working off of the db.session object, but it's relying on the older Query model that is going to be phased out in future releases of SQLAlchemy. This is an opportunity to start leveraging the current and newer approach now available to us with the 2.0.x upgrade! 🙂
Please take a look at the guide to see how to write the newer style queries with the select method syntax. The guide also includes documentation on how to add WHERE and GROUP BY clauses with the ORM as well, so all of the pieces are in place. Please let @xlorepdarkhelm or I know if you have any questions or are unsure how to approach this, we're happy to help!
Everything else looks good to me here. The endpoints all look great, there's error handling in place for the values being used as parameters, tests are updated, and everything is organized well. Thanks for working closely with @heyitsmebev on this, it's a great example of team collaboration! 🙂
app.utils utc_now() function. Added new endpoint
/service/{{service_id}}/notifications/month
This endpoint is now part of this PR. |
for single database queries to improve application performance.
standardized the SQLAlchemy calls refactored the endpoints in service/rest.py
calculation for today
altered to use NotificationAllTimeView like the other endpoints
|
Making a note here that this PR also takes care of this issue: #1008 |
ccostino
changed the title
|
@anagradova will take a final look at the requested changes here. Thank you, and let us know if you need any help or need to hand things off! 🙂 |
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
There was a problem hiding this comment.
Thank you, @anagradova! It's awesome seeing this all come together!
Thank you @xlorepdarkhelm for the assist at the end, too!
Description
This creates multiple new endpoints that are used to get statistical data:
/service/{{service_id}}/notifications/month
/service/{{service_id}}/notifications/{{user_id}}/month
/service/{{service_id}}/notifications/{{user_id}}/monthly
/service/{{service_id}}/statistics/{{start_date}}/{{day}}
/service/{{service_id}}/statistics/user/{{user_id}}/{{start_date}}/{{day}}
as well as updates to the test files to accommodate the new returned data structure in the monthly statistics return.
This PR also removes all usage of the deprecated datetime.utcnow() function and replaces it with the app.utils.utc_now() function.
This should include the following issues:
#1008
#1007
#1006
No dependency changes
TODO (optional)
Security Considerations
These are new endpoints that accept parameters from authorized clients. They should have additional security testing to make sure any SQL injection or other parameter risks are being handled by SQLAlchmey and the code properly.