Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
🎉 Homepage redesign #3219
🎉 Homepage redesign #3219
Changes from 37 commits
f7d13c0
e43481b
e9cdb18
1f27abb
99ec19b
65473c6
593867c
48f8cea
34d433f
9dbf807
127fb5a
2830138
adf0c83
daa1fec
2ecb6fd
b5f3552
0296eb6
e2d5d4c
f6c1501
1ed2832
2211b41
a8a539f
f3259c4
08e6583
251d6bc
e0871d2
3c90cf8
ad28f39
5b4abf1
ca36eee
a56660a
43945d1
8efc438
5dc0515
4865899
27f20ee
20739c2
622b391
8078282
1be6bbf
cf8fc5f
cbb078e
d069b34
37b8a3a
a59782d
a0ffe43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The modifications to the
renderFrontPage
function introduce a new approach to rendering the homepage by checking for a published homepage Google Doc before proceeding with the existing logic. This change is significant as it affects the initial content users will see. Consider the following points:db.knexRawFirst
uses parameterized inputs, which is a good practice for preventing SQL injection vulnerabilities. However, since this is a critical security aspect, double-check that all dynamic inputs to SQL queries throughout the application follow this practice.Overall, the approach seems sound, but careful attention should be paid to security, error handling, and performance implications.
Consider implementing caching for the homepage check or rendered content to improve performance. Additionally, ensure robust error handling around the database query and Google Doc loading process.
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.
seems a bit heavy handed, given that we can get the index in the
map()
below (EDIT: I'm seeing now this has been merely moved so not really part of this PR)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.
with the recent refactor in #3194, you're now able to get the knex instance from the baker directly
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.
Just making sure I understand you - you mean all these db functions should take knex as an argument and we pass the same instance around everywhere? 🙂
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.
yep 😉
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.
The
getLatestDataInsights
function retrieves the latest data insights with a limit parameter. It's well-structured and uses parameterized queries to prevent SQL injection. However, consider adding error handling to manage potential database query failures gracefully.Committable suggestion
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.
The
getPublishedDataInsightCount
function correctly counts the number of published data insights. It uses parameterized queries, which is good for security. Similar to the previous function, consider adding error handling for robustness.Committable suggestion
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.
The
getTotalNumberOfCharts
function is straightforward and correctly counts the total number of published charts. It also uses parameterized queries for security. Again, adding error handling would improve the robustness of this function.Committable suggestion
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.
The
getTotalNumberOfTopics
function correctly counts the number of unique topics by counting distincttagId
values fromchart_tags
where the chart is published. It follows best practices for query parameterization. Similar to other functions, adding error handling would enhance its reliability.Committable suggestion