-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor: database calls for homepage #351
Conversation
owner_name = db.execute_return( | ||
"SELECT name FROM users WHERE id = %s", [res[2]] | ||
) |
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.
Why don't we add this to the complex SQL query as well?
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.
Yeah we can
projects_query = ( | ||
sql.SQL("WITH LikesSummary AS (") | ||
+ likes_query | ||
+ sql.SQL("),") | ||
+ sql.SQL(" CombinedProjects AS (") | ||
+ PROJECTS_BASE_QUERY | ||
+ sql.SQL(") ") | ||
+ sql.SQL( | ||
"SELECT cp.*, COALESCE(ls.total_likes, 0) AS total_likes FROM" | ||
" CombinedProjects cp LEFT JOIN LikesSummary ls " | ||
" ON cp.uuid = ls.project_uuid ORDER BY total_likes DESC " |
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.
Can't we make this whole thing the project query? We would have to add likes to the project but maybe we want that anyway...
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.
In future we will sort by different things so want to keep it separate. Same for calculating the count don't need to sort?
likes_query = sql.SQL( | ||
"SELECT report_id, COUNT(*) as total_likes" | ||
" FROM report_like GROUP BY report_id " | ||
) | ||
reports_query = ( | ||
sql.SQL("WITH LikesSummary AS (") | ||
+ likes_query | ||
+ sql.SQL("),") | ||
+ sql.SQL(" CombinedReports AS (") | ||
+ REPORTS_BASE_QUERY | ||
+ sql.SQL(") ") | ||
+ sql.SQL( | ||
"SELECT cp.*, COALESCE(ls.total_likes, 0) AS total_likes FROM" | ||
" CombinedReports cp LEFT JOIN LikesSummary ls " | ||
" ON cp.id = ls.report_id ORDER BY total_likes DESC " | ||
) |
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.
Same as above.
) | ||
else: | ||
res = db.connect_execute_return( | ||
sql.SQL("SELECT COUNT(*) FROM") + PROJECTS_BASE_QUERY + sql.SQL(";"), |
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.
This might speak against including likes. Don't know.
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.
Yup!
@@ -513,41 +505,41 @@ def get_projects(current_user=Depends(auth.claim())): | |||
user = select.user(current_user["username"]) | |||
if user is None: | |||
return Response(status_code=status.HTTP_401_UNAUTHORIZED) | |||
return select.projects(user) | |||
return select.projects(user, ProjectsRequest()) |
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.
Why don't we make this a param of the function as well? What if I want limits here, too?
@@ -558,7 +550,7 @@ def get_reports(current_user=Depends(auth.claim())): | |||
user = select.user(current_user["username"]) | |||
if user is None: | |||
return Response(status_code=status.HTTP_401_UNAUTHORIZED) | |||
return select.reports(user) | |||
return select.reports(user, ReportsRequest()) |
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.
Same as above.
We currently don't need them for these calls (from reports) - I think we can add it here if we need to in the future. |
Description
Better DB calls for backend, base for pagination on homepage.