Conversation
- FEATURE: added new contact page based on figma mockup - FEATURE: added contact page to __manifest__.py
- REFACTOR: Removed second title component and changed the text of the first one
- FEAT: added s_website_xxx classes to input fields - FEAT: added the csrf_token to the form - FEAT: added an optional add_class to the ThemedButton
- REFACTOR: Created a new contact us thank you page - REFACTOR: Changed the data-success-page url in the contact us form - FEAT: Added new translation terms to fr/de/it .po files - STYLE: pre-commit indentation
Summary of ChangesHello @Danielgergely, 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 introduces a modern, themed contact page and a corresponding thank you page, enhancing the user experience for inquiries. The changes ensure that users can easily get in touch and receive confirmation of their message submission, with full support for internationalization across multiple languages. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new contact page and a corresponding 'thank you' page with a fresh design for the 2025 theme. The changes also include adding an add_class parameter to the themed button component for more flexibility.
My review has identified a few issues, primarily in the new contact form (my2_contact_us.xml):
- There are problems with how form fields are named and mapped to the backend model, which will lead to data loss and server-side warnings.
- Several required fields are missing the necessary class to indicate they are mandatory, which impacts user experience.
- The new translation strings for German, French, and Italian have been added but are empty.
I've provided specific comments and suggestions to address these points. Overall, the new pages are a good addition once these issues are resolved.
I am having trouble creating individual review comments. Click here to see my feedback.
my_compassion/templates/pages/my2_contact_us.xml (53-117)
There are a few issues with the form fields mapping to the crm.claim model:
- The hidden input for the full name has
name="Your Name". This is not a valid field oncrm.claimand will be ignored. It should bepartner_name, which is handled bywebsite_formto set the contact's name. - The
title,firstname, andlastnameinputs havenameattributes that do not correspond to fields oncrm.claim. This will cause warnings on the server. Assuming thejs_combine_namescript combines these into the hiddenpartner_namefield, you should remove thenameattributes from thefirstnameandlastnameinputs. For thetitleradio buttons, you should use a name that won't conflict with model fields (e.g.,title_radio) to maintain their grouping behavior.
<input
type="hidden"
name="partner_name"
id="contact_us_full_name"
class="s_website_form_input"
value=""
/>
<!-- Title Mr. or Mrs -->
<t t-call="theme_compassion_2025.FormFieldComponent">
<t t-set="top_class" t-value="'col-12 mb-3'" />
<div class="form-check form-check-inline">
<input
class="form-check-input js_combine_name"
type="radio"
name="title_radio"
id="mrs"
value="Mrs."
/>
<label class="form-check-label text-pure-white" for="mrs">Mrs.</label>
</div>
<div class="form-check form-check-inline">
<input
class="form-check-input js_combine_name"
type="radio"
name="title_radio"
id="mr"
value="Mr."
/>
<label class="form-check-label text-pure-white" for="mr">Mr.</label>
</div>
</t>
<!-- First Name and Last Name -->
<t t-call="theme_compassion_2025.FormFieldComponent">
<t t-set="label">First Name</t>
<t t-set="label_color" t-value="'pure-white'" />
<t
t-set="top_class"
t-value="'col-12 col-md-6 mb-3 s_website_form_field s_website_form_required'"
/>
<t t-set="input_id" t-value="contactus_form_firstname" />
<input
id="contactus_form_firstname"
type="text"
class="form-control js_combine_name s_website_form_input"
placeholder="First Name"
required="required"
/>
</t>
<t t-call="theme_compassion_2025.FormFieldComponent">
<t t-set="label">Last Name</t>
<t t-set="label_color" t-value="'pure-white'" />
<t
t-set="top_class"
t-value="'col-12 col-md-6 mb-3 s_website_form_field s_website_form_required'"
/>
<t t-set="input_id" t-value="contactus_form_lastname" />
<input
id="contactus_form_lastname"
type="text"
class="form-control js_combine_name s_website_form_input"
placeholder="Last Name"
required="required"
/>
</t>
my_compassion/templates/pages/my2_contact_us.xml (123)
The 'Phone number' field is marked as required on the input element, but its container FormFieldComponent is missing the s_website_form_required class. This class is necessary for Odoo's website form logic to display the required field indicator (*). This inconsistency can confuse users. This issue also applies to the 'E-mail', 'Subject', and 'Message' fields.
<t t-set="top_class" t-value="'col-12 col-md-6 mb-3 s_website_form_field s_website_form_required'" />
my_compassion/i18n/de.po (1131-1210)
The newly added translation strings for German are currently empty. This will result in untranslated text on the new contact and thank you pages for German-speaking users. This also applies to the fr_CH.po and it.po files. Please either add the translations or ensure a follow-up task is created for the translation team.
my_compassion/templates/pages/my2_contact_us.xml (125-132)
The input for the phone number is of type="text". Using type="tel" is more semantically correct for a phone number field. This can improve user experience on mobile devices by showing a numeric keypad.
<input
id="contactus_form_phone_number"
type="tel"
name="partner_phone"
class="form-control s_website_form_input"
placeholder="0041244342124"
required="required"
/>
…b.com:compassionch/compassion-website into T2932-contact-page
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new contact page and a corresponding thank you page, which is a great addition. The implementation makes good use of reusable components. However, I've found a critical issue with the contact form's implementation. It's attempting to submit data to fields that do not exist on the target crm.claim model, which will cause form submissions to fail. I have added detailed comments on how to address this to ensure the form is functional.
- REFACTOR: removed unused hidden contact us full name input
NoeBerdoz
left a comment
NoeBerdoz
left a comment
There was a problem hiding this comment.
Thanks for the work!
A few things needs to be checked or redone here as you will see in my comments.
Also, on the UI side, the red upon the blue is hard to read when the user submits a form with an issue.
I would suggest to use our high-pink color instead of red.
| <input | ||
| id="contactus_form_firstname" | ||
| type="text" | ||
| name="firstname" | ||
| class="form-control s_website_form_input" | ||
| placeholder="First Name" | ||
| required="required" | ||
| /> | ||
| </t> | ||
| <t t-call="theme_compassion_2025.FormFieldComponent"> | ||
| <t t-set="label">Last Name</t> | ||
| <t t-set="label_color" t-value="'pure-white'" /> | ||
| <t | ||
| t-set="top_class" | ||
| t-value="'col-12 col-md-6 mb-3 s_website_form_field s_website_form_required'" | ||
| /> | ||
| <t t-set="input_id" t-value="contactus_form_lastname" /> | ||
| <input | ||
| id="contactus_form_lastname" | ||
| type="text" | ||
| name="lastname" | ||
| class="form-control s_website_form_input" | ||
| placeholder="Last Name" | ||
| required="required" | ||
| /> | ||
| </t> |
There was a problem hiding this comment.
The firstname and lastname fields will be ignored in my opinion because they don't exist on the crm.claim model.
Odoo relies on the standard contact_name field to automatically search for an existing Partner or create a new one to link to the Claim. Without this, SDS will receive tickets with no name attached.
There was a problem hiding this comment.
I just copied the way the old contact page was done. As mentioned in the reply to the suggestion made by GCA, the title + firstname + lastname (and any "extra" fields) will be posted as a log note attached to the request:
There was a problem hiding this comment.
Let me know if this is sufficient, or if I should combine title with first and last name and submit it like that
- REFACTOR: added dark-bg to input fields and data-validate-type attributes - FIX: name/email/phone fields now shift down if their counterpart has an error fixing the uneven display problem
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a new contact page and a corresponding thank you page, aligned with the 2025 theme. It also includes updates to the ThemedButton component to support custom classes and adds translation placeholders. While the UI structure looks solid, there are several technical issues regarding variable usage in templates, form field whitelisting, and incomplete translations that should be addressed to ensure full functionality and maintainability.
my_compassion/i18n/de.po
Outdated
| #. module: my_compassion | ||
| #: model_terms:ir.ui.view,arch_db:my_compassion.my2_contactus_page | ||
| msgid "Get in touch" | ||
| msgstr "" |
There was a problem hiding this comment.
The translation for 'Get in touch' (and several other new terms) is empty. While the terms have been added to the file, the actual translations are missing. Providing the translations would ensure the feature is fully localized as implied by the PR description.
|
The dark error messages should now be fixed. if 'dark-bg' is added to the class list of an input, then the error message should be displayed in white. This was implemented in in #253, but the inputs are adapted. As soon as both are merged a test would still be required. Here is how it should look: |
- FIX: contact us form label ids are now correctly set - FIX: contact us thank you form has correct key - FEAT: contact us translations updated
- REFACTOR: created contact us controller to fetch and inject titles from the db - REFACTOR: replaced hard coded mrs/mr input selectors to a dynamic dropdown
2 participants
A new contact and thank you for contacting us page based on the 2025 theme.