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

[16.0][IMP] edi_account_oca: add new edi_account_move_exchange_type_id field on res.partner #39

Closed

Conversation

QuocDuong1306
Copy link
Contributor

@QuocDuong1306 QuocDuong1306 commented Nov 8, 2023

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @etobella,
some modules you are maintaining are being modified, check this out!

@QuocDuong1306 QuocDuong1306 changed the title [16.0][IMP] edi_account_oca: add new edi_exchange_type_id field on res.partner [16.0][IMP] edi_account_oca: add new edi_account_move_exchange_type_id field on res.partner Nov 8, 2023
edi_account_oca/models/res_partner.py Outdated Show resolved Hide resolved
_inherit = "res.partner"

edi_account_move_exchange_type_id = fields.Many2one(
string="EDI Exchange Type For Invoices",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
string="EDI Exchange Type For Invoices",
string="EDI Exchange type for outgoing invoices",

Copy link
Member

Choose a reason for hiding this comment

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

You are not using this configuration anywhere, at least you need to add someting on the Connector 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? It has to be documented... not used.
The goal is to be able to decide which type to use to send out an invoice for that specific customer.

Copy link
Member

Choose a reason for hiding this comment

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

Which is the goal then? adding a field to use it somewhere else? Wouldn't have more sense to add the connector script?

Copy link
Contributor

Choose a reason for hiding this comment

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

@etobella what do you mean w/ "connector script"? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@etobella ping :)

@simahawk
Copy link
Contributor

Commit msg could be rewritten as
[IMP] edi_account_oca: add exc type conf field to partner

@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-imp-edi_account_oca branch from bcbc672 to 0a45ae7 Compare November 20, 2023 02:15
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-imp-edi_account_oca branch from 0a45ae7 to 50993e9 Compare November 20, 2023 02:53
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-imp-edi_account_oca branch 4 times, most recently from 5f8e987 to 81e7616 Compare January 10, 2024 03:36
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-imp-edi_account_oca branch from 81e7616 to e1ba065 Compare January 10, 2024 03:37
class ResPartner(models.Model):
_inherit = "res.partner"

edi_account_move_out_exchange_type_id = fields.Many2one(
Copy link
Member

Choose a reason for hiding this comment

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

But where are you using it? shouldn't you define it on a component or something like that? Also... I would define several exchange types and make a filter for it

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But where are you using it?

This is not relevant. You use it whenever you want to send out an invoice.

shouldn't you define it on a component or something like that?

Having an hardcoded type in the listener? That's not what you want.
Plus, you might want to customize the exchange type by partner.
This can also be the base to replace or integrate the transmit method module (which is doing kind of the same thing: define on the partner how you want to send invoices).

TBH I don't get what's so terrible in having an explicit configuration per partner to send out invoices :)

Copy link
Member

Choose a reason for hiding this comment

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

Why is not relevant? I mean, if we are adding a field with no more interest than use it somewhere else, maybe we need to define it in the relevant module, isn't it? Also, if we use a similar component in two different modules to define the usage of this field, we might send the invoice twice, isn't it? IMO, we should define the field here if we define the logic, otherwise, I cannot find a justification.

Another though, in my experience, I have found some customers that require to send the information in two different ways. It might have sense to make it Many2many

Copy link
Member

Choose a reason for hiding this comment

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

Also, it might have sense to define a tuple: company, backend, exchange type, because we might use the same exchange type for several backend's, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we are adding a field with no more interest than use it somewhere else, maybe we need to define it in the relevant module, isn't it?

No. This is exactly what we do in base modules. This one IS a base module for EDI framework + invoice.
Hence, from my POV it makes perfectly sense to keep it here.

we might use the same exchange type for several backend's, isn't it?

IMO if you assign a type to a partner it must have a backend.

Regarding an advanced configuration: we should think properly about it. M2M won't be enough.
For a more granular thing we probably need an model like the type rule that would allow you to configure different behaviors. If we do it, we can also replace the transmit method stuff (which is only for sending).

However, this will take quite some time to implement. IMO for this basic scenario you can rely on this field and add a new one per each additional exchange you want to send (like in your case) or keep it hardcoded if you prefer.
Meanwhile, we can create an RFC and define specs and roadmap for this improvement.
We can also make a call - as usual - to clarify all the aspects.

Sounds good?

Copy link
Member

Choose a reason for hiding this comment

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

@simahawk Let me show you an example:

In spain we have a format for Electronic invoicing called Facturae and can be delivered in several ways FACe, FACeB2B, e.Fact.... We are reusing the same exchange type, because the format is exactly the same. For that reason, I was talking of exchange type and backend. This field could not be used in this case.

Also, some customers require to send the Facturae file in the standard way and another format to verify all the data with extra information. We know that customers usually like to manage it in the hard way. another problem arises, this standard field cannot be used, because we need to send in two different ways.

I was suggesting to add a table with 3 fields partner_id, exchange_type_id and backend_id
and a listener with a code like:

# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from odoo.addons.component.core import Component


class AccountMoveL10nEsFacturaeListener(Component):
    _name = "account.move.edi.listener"
    _inherit = "base.event.listener"
    _apply_on = ["account.move"]

    def on_post_account_move(self, records):
        for record in records:
            if record.edi_disable_auto:
                continue
            partner = record.partner_id
            # TODO: maybe we need to manage it in the table too...
            if record.move_type not in ["out_invoice", "out_refund"]:
                continue
           for configuration in partner.edi_account_configuration_ids:
               exchange_type = configuration.exchange_type_id
               backend = configuration.backend_id or configuration.exchange_type_id.backend_id
               if record._has_exchange_record(exchange_type, backend):
                    continue
                exchange_record = backend.create_record(
                    exchange_type.code, self._get_exchange_record_vals(record)
                )
                exchange_record.with_delay().action_exchange_generate()

I think it is not too hard.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap up notes of our call...
We could have a generic edi.configuration model with these characteristics:

    description: describe what the conf is for
    backend_id
    type_id

    model_id: model the conf applies to
    res_id: id of the record it belongs to
        model_id an res_id will be set automatically in the specific form/ctx.
        The idea is that we can reuse configurations for other records (eg: apply the same conf to several customers)
        We'll have a m2m from consumer records rather than an o2m.

    target: Selection field to make easier and more explicit the use of the conf for a specific event (eg: on_post_account_move, on_so_confirm, etc)

    snippet_validate_todo: validate the state, used to collect records todo
        receives: operation, edi_action, vals, old_vals
        no need for triggers conf nor "if", "callable", etc, anymore
        the snippet can return
            todo = True/False
            snippet_do_vars = {}  # set of variables to pass over to the next snippet
            event_only
            tracked_fields
            .. in fact, any other variable that might be needed
            Then if we use `snippet_do` instead of assuming what has to be done... 
            all these vars become useless.

        NOTE: this makes sense only in the ctx of `on_create` and `on_write` because otherwise there's no need to collect records todo beforehand.
        You can simply use `snippet_do` otherwise.
        We could maybe encourage the use of specific targets instead in a help text.

    snippet_do: 
        Do something specific. 
        In the ctx of *_auto it can be used by `_edi_auto_handle` to execute what you want.
        Receives full EDIAutoInfo like obj (specific keys to be removed, add snippet_do_vars as attr)

Specific modules will then be able to define:

  • the hooks (listeners) and register new target options
  • specific fields for configuration (eg: edi_account_configuration_ids) so that we can show some conf only to specific groups

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 18, 2024
@github-actions github-actions bot closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants