Skip to content
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

[FIX] project: create projects linked to company-specific customer #162961

Draft
wants to merge 1 commit into
base: saas-16.4
Choose a base branch
from

Conversation

naja628
Copy link
Contributor

@naja628 naja628 commented Apr 23, 2024

[FIX] project,hr_timesheet: create projects with company-specific customer

Problem

When creating projects with a customer whose company field is set,
_check_company raises an Error because of a mismatch with
the analytic account's company (False).

Steps

(from fresh db, with demo data)

  1. install project and sale_management (so that projects can be
    billable)
  2. in the settings, create a second company,
    (so that contacts' 'Company' field can be set)
  3. on a contact, set the 'Company' field (under: Sales & Purchase > Misc)
    to the current company ('YourCompany')
  4. In Setting > Project: enable 'Timesheets'
  5. Try to create a new billable project with 'Timesheets' enabled
    and the customer from step 3

Fix

Never set the partner on the analytic account when creating a new
project.
Note that we have to modify the corresponding test.
This is acceptable because in some flows (create project with
no partner, then set the partner) this is what
already happens and seems to not be a problem.

opw-3865150


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Apr 23, 2024

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 23, 2024
@naja628 naja628 force-pushed the 17.0-opw-3865150-billable_project_creation_error-naja branch 2 times, most recently from 63dd145 to 86aaca8 Compare April 25, 2024 08:55
@naja628 naja628 changed the base branch from 17.0 to saas-16.4 April 25, 2024 08:55
@naja628
Copy link
Contributor Author

naja628 commented Apr 25, 2024

This fix changes the internal logic more than I'd like ideally, but I think we're a bit stuck if we want the company_id field to be optional on project (as per 16a07ed), since it looks like:

  • analytic accounts are sometimes required, especially when we have billable timesheets
  • when we have both a partner and an analytic account their companies must match even when False

@adwid adwid requested a review from thle-odoo April 25, 2024 13:07
@thle-odoo
Copy link
Contributor

There is currently a task for the R&D team to solve this behaviour (3757184)

@naja628 naja628 force-pushed the 17.0-opw-3865150-billable_project_creation_error-naja branch 3 times, most recently from 940888a to d78a8dc Compare May 15, 2024 10:15
@naja628
Copy link
Contributor Author

naja628 commented May 15, 2024

Another possible fix if we do want the analytic account's partner_id to be set would be the following diff in:

def _create_analytic_account_from_values(self, values):

+       save_company = False
+       if not company and (partner_id := values.get('partner_id')):
+           partner = self.env['res.partner'].browse(partner_id)
+           save_company = partner.company_id
+           partner.write({'company_id': False})

        analytic_account = self.env['account.analytic.account'].create({
            'name': values.get('name', _('Unknown Analytic Account')),
            'company_id': company.id,
            'partner_id': values.get("partner_id", False),
            'plan_id': plan.id,
        })

+       if save_company:
+           partner.write({'company_id': save_company.id})

Where the idea is that we temporarily unset the partner's company to circumvent the check.

I think with this fix the runbot is fully green without having to editing existing tests, but I'm not choosing it as a first choice because of the code smell.

Also in this case we probably also need to somehow ensure the analytic account's partner is set alongside the project's if we want to be consistent.

Yet again a 3rd possibility would be to enforce the constraint that the project's company has to match its partner's.

@naja628 naja628 force-pushed the 17.0-opw-3865150-billable_project_creation_error-naja branch 2 times, most recently from 1eab0b4 to e143298 Compare May 15, 2024 10:26
Problem
---
When creating projects with a customer whose company field is set,
`_check_company` raises an Error because of a mismatch with
the analytic account's company (False).

Steps
---
(from fresh db, with demo data)
1. install `project` and `sale_management` (so that projects can be
   billable)
2. in the settings, create a second company,
   (so that contacts' 'Company' field can be set)
3. on a contact, set the 'Company' field (under: Sales & Purchase > Misc)
   to the current company ('YourCompany')
4. In Setting > Project: enable 'Timesheets'
5. Try to create a new billable project with 'Timesheets' enabled
   and the customer from step 3

Fix
---
Never set the partner on the analytic account when creating a new
project.
Note that we have to modify the corresponding test.
This is acceptable because in some flows (create project with
no partner, **then** set the partner) this is what
already happens and seems to not be a problem.

opw-3865150
@naja628 naja628 force-pushed the 17.0-opw-3865150-billable_project_creation_error-naja branch from e143298 to ebe8b65 Compare May 15, 2024 10:27
Copy link
Contributor

@thle-odoo thle-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @naja628,

I think the different solutions can work (it's up to the R&D team to decide which one suits them best).

The solution of not setting the partner on the analytic_account (current diff) may be radical, but it allows us to fallback on the behavior we know when we create a project and set the partner afterwards.

The second solution, to remove the company and then put it back, bypasses the company check, but I don't think it's a good practice.

The third solution of reinforcing the constraint may not be the best solution for stable versions.

@xavierbol What do you think ?

@naja628
Copy link
Contributor Author

naja628 commented Jun 4, 2024

@xavierbol Hello :)
Just a friendly ping since this PR has been dormant for a while
Edit: just saw you were off this week sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants