-
Notifications
You must be signed in to change notification settings - Fork 10
[T2855] - Impact trip procedure : upload criminal records #242
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
base: 14.0
Are you sure you want to change the base?
[T2855] - Impact trip procedure : upload criminal records #242
Conversation
…compute & inverse)
…emoving of rights access error)
Summary of ChangesHello @SlWa99, 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 a critical bug in the MyCompassion portal that prevented users from successfully uploading Criminal Records and Passports, particularly when event registrations were in an 'Attended' state. The changes ensure secure and reliable document uploads by standardizing document management, leveraging appropriate security contexts for write operations, and simplifying the underlying data model. This significantly improves system stability, prevents unintended status changes, and enhances the overall user experience for document submission. 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 effectively addresses the critical issue of failing criminal record uploads by refactoring the document handling logic. Moving the passport and criminal_record fields to res.partner and using related fields on event.registration is a great simplification that improves data integrity and maintainability. The use of sudo() to ensure documents are saved correctly from the portal is a pragmatic solution to the access rights issue.
My review highlights a potential robustness issue with the implementation in the wizard that could cause the fix to be incomplete, and also points out some leftover code from the refactoring that should be removed to keep the codebase clean. Addressing these points will make the solution more robust and maintainable.
I am having trouble creating individual review comments. Click here to see my feedback.
website_event_compassion/wizards/registration_form.py (38)
While this sudo().write() call is the correct way to save with elevated privileges, there's a potential issue with the overall approach. Because the passport and criminal_record fields on this wizard are defined as related fields, the Odoo ORM will attempt to write their values during the super().create() call. This initial write does not use sudo() and is likely to fail with an access error, which might be the root cause of the crashes mentioned in the PR description.
For a more robust fix, I recommend changing the passport and criminal_record fields on the event.registration.form model to be standard fields.Binary() instead of related fields. This would prevent the initial, non-privileged write attempt and ensure this sudo() write is the only one performed.
website_event_compassion/models/event_registration.py (139-141)
This refactoring is a good improvement. As part of this change, the _compute_passport method is now dead code and should be removed to improve maintainability and prevent future confusion. The method also references passport_filename, which no longer exists, and would raise an error if ever called.
|
Feedback applied. I have removed the related attribute from the passport and criminal_record fields in the wizard. As pointed out, the ORM was attempting a non-privileged write to the partner during super().create(), causing an access error when the registration was in 'Attended' status. By using standard Binary fields, the files are now stored as a buffer in the transient model and then explicitly saved to the partner using sudo().write() in the create method. |
ecino
left a comment
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.
Please bring the changes from CompassionCH/compassion-modules#2065 inside this module as well.
|
Ok, it's normally done ! |
Criminal Record Save Failure Resolution
Purpose and Context
The goal of this PR is to fix a critical issue preventing users from updating their Criminal Record via the MyCompassion portal.
Due to Odoo security constraints and inconsistent document handling, uploads were failing when registrations were in the Attended state. In some cases, this also caused unstable registration statuses and the disappearance of events from the My Events section.
This PR should be reviewed together with a related PR, as both contribute to stabilizing the document upload and event registration workflows.
Applied Changes and Improvements
1. Portal Security and Save Reliability
Secure write with
sudoDocument saving in
event.registration.formnow usesform.registration_id.sudo().write(docs_to_save).Result
Documents are saved with sufficient privileges, preventing access errors, redirection crashes, and unintended status changes. The Attended status is preserved.
2. Document Management Standardization
Unified handling
Passport and Criminal Record are now managed in an identical and consistent way.
Technical debt removal
Complex
compute/inverselogic for Passport has been removed.Both documents are now standard
Binaryfields withattachment=Trueonres.partner.Single source of truth
Documents are physically attached to the partner (contact).
Event registrations access them through related fields.
XML cleanup
Backend views were simplified by removing obsolete technical fields (e.g.
passport_filename).3. Validation and Testing
Portal upload testing
Documents were uploaded using a real participant account for the Impact Trip in the Dominican Republic.
Technical verification
Uploaded documents are visible under
Settings → Technical → Attachmentsas Passport and Criminal Record.Data integrity
Each attachment correctly references the related partner in the Attached To field.
Outcome
Related PRs
This set of improvements is complemented by a related PR in the Compassion-modules repository: