T2976 Timeline ordering bugs and letters ordering bugs#281
T2976 Timeline ordering bugs and letters ordering bugs#281
Conversation
Summary of ChangesHello @Shayan105, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical display and ordering issues within the application. It rectifies the incorrect date presentation and sorting logic for letters on the dedicated letters page, ensuring a more intuitive user experience. Concurrently, it modernizes the timeline's handling of child pictures by transitioning to a notification-based system for biennial photos, which are then intelligently linked to their corresponding images. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses ordering and data display bugs in the timeline and letters pages. The changes for the timeline correctly integrate biennial photos by replacing the old child pictures logic, though the SQL queries could be optimized for better performance. For the letters page, the sorting logic is corrected to use different date fields based on the letter's direction, but this is done in Python after fetching all records, which introduces a significant performance risk. Additionally, the domain filter for letters has a critical syntax error that needs to be fixed. Finally, the fix for displaying the correct letter date in the UI is incomplete. My review includes suggestions to address these performance and correctness issues.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses two bugs related to ordering in the timeline and letters pages. The logic for timeline events for new pictures is now based on the partner communication job date, which is a good fix for the inconsistency. The letter sorting logic is also updated to use different date fields based on the letter's direction.
My review focuses on several key areas. I've identified a critical performance issue in the letter sorting implementation, which loads all records into memory instead of using database-level sorting and pagination. I've provided a suggestion to fix this using a SQL CASE statement. I also found a medium-severity performance concern in the timeline picture matching logic and suggested a more scalable approach. Finally, I've pointed out an inconsistency in SQL queries for maintainability. Addressing these points will improve the performance, scalability, and consistency of the code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
NoeBerdoz
left a comment
There was a problem hiding this comment.
As discussed, let's simplify the picture part handling to be based on the create_date
|
@NoeBerdoz Changes applied. |
Summary
This PR solves two bugs:
Problem 1
Except from updating the view, another logic is important to add.
When the letter is from the supporter to the beneficiary (S->B) then the effective date to be shown to the user is the create_date (first time the letter entered the system).
Note to Mr @ecino :
Although in the following PR you requested to use status_date only, there is a drawback in this situation. Basing the ordering of the letters S->B in the status_date can be confusing to the sponsors as this field is changing at every step of the processing of the letter. I recommend using create_date when the direction is S->B.
Problem 2
The timeline order of the picture was based on the create_date of the picture instead of taking into accoun the date the sponsor was notified by a partner_communicaiton_job "New Biennial".