-
Notifications
You must be signed in to change notification settings - Fork 51
[MIG] forward-port changes from 14.0 #2067
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
Conversation
* T2617 ADD analytic accounts on interventions - The abilty to link intervention category to funds. * Update intervention_compassion/models/compassion_intervention.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: ecino <ecino@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* Restirct notif. to only hold of type SUB_CHILD_HOLD * Run precommit * Correct wrong type of Hold * Fix typo --------- Co-authored-by: Shayan Mouktar <119674649+Shayan105@users.noreply.github.com>
Summary of ChangesHello @ecino, 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 integrates several forward-ported changes from version 14.0, focusing on enhancing financial management through analytic accounting for interventions, streamlining the gift generation workflow for contracts, and improving the robustness of communication and correspondence processes. These updates aim to provide more granular control over financial data, increase operational efficiency, and ensure more accurate system behavior in various scenarios. 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. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a forward-port of changes from version 14.0, introducing a variety of bug fixes, refactorings, and new features, notably the addition of analytic accounts for interventions. My review identifies a critical syntax issue and a potential robustness problem, offering suggestions to improve code quality and prevent runtime errors.
| for contract_id in self.contract_ids.filtered("no_birthday_invoice"): | ||
| gift_obj = self.env["sponsorship.gift"] | ||
| birthday_gift_type = self.env.ref( | ||
| "sponsorship_compassion.gift_type_birthday" | ||
| ) | ||
| gift_vals = {"sponsorship_gift_type_id": birthday_gift_type.id} | ||
| gift_date = self.compute_date_birthday_invoice( | ||
| self.contract_id.child_id.birthdate | ||
| contract_id.child_id.birthdate | ||
| ) | ||
| # Search that a gift is not already pending | ||
| existing_gifts = gift_obj.search( | ||
| [ | ||
| ("sponsorship_id", "=", self.contract_id.id), | ||
| ("sponsorship_id", "=", contract_id.id), | ||
| ("gift_date", ">=", gift_date.replace(day=1, month=1)), | ||
| ("gift_date", "<=", gift_date.replace(day=31, month=12)), | ||
| ("gift_type_id", "=", birthday_gift_type.id), | ||
| ] | ||
| ) | ||
| if not existing_gifts: | ||
| # Create a gift record | ||
| gift_vals["sponsorship_id"] = self.contract_id.id | ||
| gift_vals["sponsorship_id"] = contract_id.id | ||
| gift_vals["date_partner_paid"] = gift_date | ||
| gift_vals["gift_date"] = gift_date | ||
| gift_vals["amount"] = self.contract_id.birthday_invoice | ||
| gift_vals["amount"] = contract_id.birthday_invoice | ||
| gift_obj.create(gift_vals) | ||
| else: | ||
| super().generate_invoice(due_date) | ||
| self.contract_ids -= contract_id | ||
| super().generate_invoice(due_date) | ||
|
|
||
| def compute_date_birthday_invoice(self, child_birthdate, payment_date=None): | ||
| """Set date of invoice two months before child's birthdate""" |
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 method has a few issues:
- Syntax Error: There are indentation errors that will cause a
SyntaxErrorat runtime. For example,gift_vals = ...is not correctly indented within theifblock. - Risky Iteration: Modifying a recordset (
self.contract_ids) while iterating over it is not a safe pattern and can lead to unpredictable behavior, such as skipping records.
I suggest refactoring the method to fix the indentation and to handle the contract filtering in a safer and more readable way by first separating the contracts into two groups.
contracts_to_handle_here = self.contract_ids.filtered('no_birthday_invoice')
contracts_for_super = self.contract_ids - contracts_to_handle_here
for contract_id in contracts_to_handle_here:
gift_obj = self.env["sponsorship.gift"]
birthday_gift_type = self.env.ref(
"sponsorship_compassion.gift_type_birthday"
)
gift_vals = {"sponsorship_gift_type_id": birthday_gift_type.id}
gift_date = self.compute_date_birthday_invoice(
contract_id.child_id.birthdate
)
# Search that a gift is not already pending
existing_gifts = gift_obj.search(
[
("sponsorship_id", "=", contract_id.id),
("gift_date", ">=", gift_date.replace(day=1, month=1)),
("gift_date", "<=", gift_date.replace(day=31, month=12)),
("gift_type_id", "=", birthday_gift_type.id),
]
)
if not existing_gifts:
# Create a gift record
gift_vals["sponsorship_id"] = contract_id.id
gift_vals["date_partner_paid"] = gift_date
gift_vals["gift_date"] = gift_date
gift_vals["amount"] = contract_id.birthday_invoice
gift_obj.create(gift_vals)
self.contract_ids = contracts_for_super
super().generate_invoice(due_date)| unlist_string = id_strings.strip("[]") | ||
| object_ids += list(map(int, unlist_string.split(","))) |
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 current implementation for parsing id_strings is a bit fragile. It could fail if the string is empty or contains unexpected whitespace after stripping brackets. Using a regular expression to find all digit sequences would be more robust and handle various formats like "[1, 2, 3]", "1,2,3", or even malformed strings more gracefully.
| unlist_string = id_strings.strip("[]") | |
| object_ids += list(map(int, unlist_string.split(","))) | |
| object_ids += [int(i) for i in re.findall(r'\d+', id_strings)] |
f680ac5 to
5e09dfa
Compare
5e09dfa to
e1812f8
Compare
No description provided.