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

[14.0][IMP] edi_oca: Add new model edi.configuration #1035

Open
wants to merge 2 commits into
base: 14.0
Choose a base branch
from

Conversation

thienvh332
Copy link
Contributor

@thienvh332 thienvh332 commented Sep 6, 2024

Specs

In edi_oca

Add generic edi.configuration model with these characteristics:

Fields

active
code
  uniq identifier, normalized
description: describe what the conf is for
backend_id
type_id

model: model the conf applies to

partner_ids: Apply the conf on specific partners (Leave blank to apply all partners)

trigger: 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)
    default option: empty

snippet_before_do: validate the state, used to collect records todo
    can be used to do anything before the action that triggered it
  
    eval ctx:
        trigger = create/write/$name_of_the_event
        conf = current conf

    the snippet can return:
        todo = True/False
        snippet_do_vars = {}  # set of variables to pass over to the next snippet
        event_only
        tracked_fields
        edi_action
        .. in fact, any other variable that might be needed

snippet_do: used to do something specific here

    receives: operation, edi_action, vals, old_vals

This model can then be used by conf consumers.
Other methods:

edi_exec_snippet_before_do(self, record):
  exec before snippet, self=conf

edi_exec_snippet_do(self, record):
  exec snippet, self=conf

edi_get_conf(self, trigger, model_name=None, partners=None, backend=None):
  filter current recordset based on conditions

Backend / Exc. record

In `edi.backend._check_output_exchange_sync` move the loop on new_records to a new method `exchange_generate_send` whereas if the file has to be generated
2 jobs are chained (generate + send) if only send is needed, delay only one job.

Then add edi.exchange.record.action_exchange_generate_send and use that method from backend.
Consumer mixin

Specific fields for configuration (eg: edi_purchase_conf_ids) will be added by glue modules on res.partner.

Default conf

add generic conf for send_via_email

code: send_via_email
trigger: on_email_send
snippet: `record._edi_send_via_email()

add generic conf for send_via_edi

code: send_via_edi
trigger: none
snippet: `record._edi_send_via_edi(conf.type_id)

NOTE: sending emails is not really bound to edi
but in real life it's handy to have a default way to send EDI docs via email.
You could even generate an exchange record to keep track of that
but the choice will be up to the implementers.
The key is that w/ this features they can do whatever they want.

Views

add a specific menu item in edi settings to see all confs
make them searchable by model, active/inactive, partner, exc type, trigger

On edi_purchase_oca

add conf trigger on_button_confirm_purchase_order

add res.partner.edi_purchase_conf_ids
domain model=purchase.order, default_model=purchase.order, default_res_id=active_id

(document this approach in edi_oca readme section for edi.conf)

add generic conf for send_via_email_rfq

code: send_via_email_rfq
snippet: `record._edi_send_via_email(ir_action=record.action_rfq_send())

Example of specific listener implementation:

class EDIConfigPurchaseListener(Component):
    _name = "edi.config.consumer.listener.purchase.order"
    _inherit = "base.event.listener"

def on_button_confirm_purchase_order(self, record):
    trigger = "on_button_confirm_purchase_order"
    confs = record.edi_purchase_conf_ids.edi_get_conf(trigger)
    for conf in confs:
        conf.edi_exec_snippet_do(record)

@OCA-git-bot
Copy link
Contributor

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

@etobella
Copy link
Member

etobella commented Sep 6, 2024

More information would be awesome in order to understand why you did this.

@simahawk
Copy link
Contributor

simahawk commented Sep 6, 2024

@etobella this origins from our discussion on OCA/edi-framework#39
@thienvh332 I'm adding the specs I gave you in the description feel free to adapt / add more things.
IMO we should split the purchase part when ready (let's say before switching from draft to real PR to ease the final review of both parts).

@simahawk
Copy link
Contributor

simahawk commented Sep 6, 2024

@thienvh332 as it's draft, do you want us to wait for a review?

@nilshamerlinck
Copy link
Contributor

hi @simahawk

  • yes, please wait before reviewing, we need to finalize the internal review on our side
  • now that you confirmed you want this in v14, we'll complete it asap

@thienvh332 thienvh332 force-pushed the 14.0-imp-edi_oca branch 8 times, most recently from 89cc982 to 15a994e Compare September 18, 2024 10:55
@thienvh332 thienvh332 marked this pull request as ready for review September 18, 2024 11:11
@thienvh332
Copy link
Contributor Author

Hello @simahawk ,
This PR is ready for review. Could you please take a look at it?

@simahawk
Copy link
Contributor

simahawk commented Oct 9, 2024

can you drop the pypdf fix? 15a994e

):
# Default action if not provided
if ir_action is None:
# `action_send_email` is just an action name I created
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get what's the need for this ... there's no action_send_email anywhere.

code = fields.Char(required=True, copy=False, index=True, unique=True)
description = fields.Char(help="Describe what the conf is for")
backend_id = fields.Many2one(string="Backend", comodel_name="edi.backend")
type_id = fields.Many2one(
Copy link
Contributor

Choose a reason for hiding this comment

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

type should be mandatory

Copy link
Contributor

Choose a reason for hiding this comment

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

no, as per our comments.
Please add a comment here explaining why it is not mandatory ;)

edi_oca/models/edi_configuration.py Outdated Show resolved Hide resolved
edi_oca/models/edi_configuration.py Outdated Show resolved Hide resolved
edi_oca/models/edi_configuration.py Show resolved Hide resolved
edi_oca/components/base_listener_config.py Outdated Show resolved Hide resolved
edi_oca/models/edi_configuration_mixin.py Outdated Show resolved Hide resolved
edi_oca/tests/fake_models.py Outdated Show resolved Hide resolved
edi_oca/tests/fake_models.py Outdated Show resolved Hide resolved
edi_purchase_oca/components/listener_purchase_order.py Outdated Show resolved Hide resolved
@thienvh332
Copy link
Contributor Author

Hi @simahawk ,
I have updated the PR but I have 2 things I need your opinion on. Could you give me your views?

I don't get what's the need for this ... there's no action_send_email anywhere.

  • We have a common config send_via_email. There we don't know what ir_action is used on each model. So I created it as a common path for other models. They can use _edi_send_via_email without passing ir_action parameter.
  • Of course, if you feel it is inappropriate I will delete it.

type should be mandatory

  • About type_id must be mandatory:
    • I'm confused about this. There is a requirement to create 2 common confs (send_via_email and send_via_edi). If type_id is required then we will:
      • Create data for edi_exchange_type and related models in edi_oca module.
      • Create those 2 common confs in another module to avoid creating unwanted data when installing the edi_oca module.

@simahawk
Copy link
Contributor

Hi @simahawk , I have updated the PR but I have 2 things I need your opinion on. Could you give me your views?

I don't get what's the need for this ... there's no action_send_email anywhere.

* We have a common config **`send_via_email`**. There we don't know what **`ir_action`** is used on each model. So I created it as a common path for other models. They can use **`_edi_send_via_email`** without passing **`ir_action`** parameter.

* Of course, if you feel it is inappropriate I will delete it.

The idea is that everything should be explicit. You must explicitly declare which action you want to use.

type should be mandatory

* About **`type_id`** must be mandatory:
  
  * I'm confused about this. There is a requirement to create 2 common confs (**`send_via_email`** and **`send_via_edi`**). If **`type_id`** is required then we will:
    
    * Create data for **`edi_exchange_type`** and related models in **`edi_oca`** module.
    * Create those **`2 common confs`** in **`another module`** to avoid creating unwanted data when installing the **`edi_oca`** module.

Ok, you are right. Cannot be mandatory. Good point :)

def exchange_generate_send(self, recordset, skip_generate=False, skip_send=False):
for rec in recordset:
if skip_generate:
job1 = rec
Copy link
Contributor

Choose a reason for hiding this comment

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

what would be delayed in this case if we skip_send too? 🤔

auto_join=True,
index=True,
)
model = fields.Many2one(
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
model = fields.Many2one(
model_id = fields.Many2one(

[
("on_record_write", "Update Record"),
("on_record_create", "Create Record"),
("on_email_send", "Send Email"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is to ease the configuration for models like SO, PO, INV, that can be sent by email.
As there's no specific implementation here, I would add a comment that explain why this is here.

Also, I think trigger should be mandatory otherwise the conf is useless, no?
Maybe we can add another option like ("disabled", "Disabled") which would allow to keep the conf visible but disabled.


def edi_get_conf(self, trigger, backend=None):
domain = [("trigger", "=", trigger)]
backend_ids = self.mapped("type_id.backend_id.id")
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point on filtering on all the backends assigned to the current recordset?

code = fields.Char(required=True, copy=False, index=True, unique=True)
description = fields.Char(help="Describe what the conf is for")
backend_id = fields.Many2one(string="Backend", comodel_name="edi.backend")
type_id = fields.Many2one(
Copy link
Contributor

Choose a reason for hiding this comment

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

no, as per our comments.
Please add a comment here explaining why it is not mandatory ;)

def _edi_send_via_edi(self, exchange_type):
exchange_record = self._edi_create_exchange_record(exchange_type)
exchange_record.action_exchange_generate_send()
msg = _("EDI auto: output generated.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not only generated but generated and sent.
I wonder if we need this custom message...
The backend will call notify_action_complete when is done...


def on_button_confirm_purchase_order(self, record):
trigger = "on_button_confirm_purchase_order"
confs = record.mapped("partner_id.edi_purchase_conf_ids").edi_get_conf(trigger)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to use mapped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants