[14.0][IMP] create subscription journal per company #75
Conversation
| continue | ||
| subscription_request_model.create_journal(company) | ||
| # this is called here to support the first 2 cases explained above. | ||
| self._init_cooperator_demo_data() |
There was a problem hiding this comment.
I'm confused by the demo word here. Why demo? The first 2 cases can happen in production, right?
Did you mean something like default?
There was a problem hiding this comment.
i understand your confusion. indeed, it is about actual demo data, and this will be called in production, but _init_cooperator_demo_data() has an early return in case demo data is disabled, and it has no effect in that case. do you see another way to support the case where a database is initialized with cooperator and demo data but no l10n module? or do you have a suggestion for a comment explaining the situation better maybe?
There was a problem hiding this comment.
Maybe it's sufficient to define the demo data for company 1? Or does the demo data journal get erased that way because of the chart being loaded afterwards?
There was a problem hiding this comment.
there is no demo journal (it is the regular one), but indeed, the demo cooperator account (which must be created manually without demo data) gets erased when loading the account chart template. this is why it is created from here.
| _logger.warning( | ||
| "Multiple companies found. " | ||
| "Please set the subscription journal manually for companies with " | ||
| "ids: {company_ids}".format(company_ids=company_ids[1:]) |
There was a problem hiding this comment.
Until I noticed the [1:] part, I wasn't understanding the code.
Shouldn't we add some comment saying that we'll migrate only the first company leaving the rest without a journal?
There was a problem hiding this comment.
you’re right, a comment is needed here. instead of adding a comment, i’ve changed the migration script to handle multiple companies with a warning in case the subscription journal is not found. what do you think?
cooperator/__init__.py
Outdated
| # the subscription journal must be created for each company. | ||
| env = api.Environment(cr, SUPERUSER_ID, {}) | ||
| subscription_request_model = env["subscription.request"] | ||
| for company in env.companies: |
There was a problem hiding this comment.
Is env.companies always equal to env["res.company"].search([])?
There was a problem hiding this comment.
no, indeed, i’ve just checked and it can be different (it depends on the company_ids field of the superuser, which can contain any company, not necessary all of them). i’ve fixed it while converting this post-init hook to a function call from the xml data file.
| continue | ||
| subscription_request_model.create_journal(company) | ||
| # this is called here to support the first 2 cases explained above. | ||
| self._init_cooperator_demo_data() |
There was a problem hiding this comment.
Maybe it's sufficient to define the demo data for company 1? Or does the demo data journal get erased that way because of the chart being loaded afterwards?
| "account_id": self.company.property_cooperator_account.id, | ||
| "account_id": self.env.company.property_cooperator_account.id, |
There was a problem hiding this comment.
Explicit is better than implicit. Let's define a company attribute in the set-up method that is equal to self.env.company so that you can use self.company here.
There was a problem hiding this comment.
since we’re also testing the multi-company behavior here (also possibly when calling test methods), do we really want to reference one particular company from the class? i removed the reference to it because i thought that it wasn’t useful. i’ve re-added it, we’ll see with later cases how it will turn out.
There was a problem hiding this comment.
I think it's usually useful to have a 'default' record in tests. That doesn't preclude you from using/creating a non-default record in several test methods.
cooperator/tests/test_cooperator.py
Outdated
| Test that creating a new company creates a new subscription journal | ||
| for it. | ||
| """ | ||
| company_1 = self.env.company |
There was a problem hiding this comment.
Same as previous comment, for all instances of self.env.company in this file.
| cr.execute( | ||
| """ | ||
| delete | ||
| from ir_model_data | ||
| where id = %s | ||
| """, | ||
| (xml_id,), | ||
| ) |
There was a problem hiding this comment.
Question: does Odoo not do this itself after updating the module?
There was a problem hiding this comment.
i tested it and no, it does not. it doesn’t detect data that disappeared. it cleans up all data when uninstalling the module, though.
use xml data instead of post-init hook to initialize company data.
huguesdk
force-pushed
the
14.0-imp-cooperator_multi_company_subscription_journal
branch
from
78ea085
to
3aae428
Compare
| @@ -44,6 +44,11 @@ def _compute_base_logo(self): | |||
| " receivable account for the" | |||
| " cooperators", | |||
| ) | |||
| subscription_journal_id = fields.Many2one( | |||
There was a problem hiding this comment.
i’m not sure about the name of this field. it is used to store the capital release request invoices/moves. any idea?
There was a problem hiding this comment.
cooperative_share_journal_id? And then rename some stuff accordingly across the module.
There was a problem hiding this comment.
after discussion with an accountant, subscription journal is a very appropriate name according to him, because once a capital release request is created, it creates a debt for the person making the subscription request and increments the capital of the cooperative. it does not record shares movements but capital movements (linked to shares).
this is part of the current effort aiming to make
cooperatormulti-company compatible.