[18.0][IMP] edi_core_oca: add listener action#232
[18.0][IMP] edi_core_oca: add listener action#232ArnauCForgeFlow wants to merge 3 commits intoOCA:18.0from
Conversation
0bc4d2e to
7a14077
Compare
|
@simahawk Thanks for the review! So, basically, your suggestion is moving the helper method _trigger_edi_event_make_name from the component module to the core, and then simply calling the _trigger_edi_event hook from the other modules? |
yes. And we should move the hook to the consumer mixin so that the logic won't be put into generic models like exc record. |
The problem with moving the hook to the consumer mixin is that for input exchanges that fail early, there is no record_id yet. For example, in edi_storage_oca, even if the process fails, we still need to execute the hook to move the file from 'pending' directory to 'error' directory. Any idea on how to handle this? Thanks! |
|
@ArnauCForgeFlow good point. Well, that's why components have been used so far 😄 A possible approach is the one I took on endpoint_route_handler to check cross models that we wouldn't have any routing conflict. See https://github.com/OCA/web-api/blob/18.0/endpoint_route_handler/models/endpoint_route_handler.py#L90 We could collect all models inheriting from consumer mixin and trigger their handlers if any. WDYT? |
|
Answering myself: this can lead to overlapping handlers as you could have the same hook for different exchanges. Could we leverage |
|
Hi @simahawk Thanks for the suggestion! I've been looking into the edi.configuration approach, but to be honest, I'm a bit lost, so I'd love a bit more context on the implementation details.
Any quick example or guidance would be appreciated. Thanks! |
|
All starts with notify_action_complete. This must be called all the times an "official" action is done. There we should trigger the event an all related records and all consumer models. Hence, for related records, we should do what is done already for state change, create and write https://github.com/OCA/edi-framework/blob/18.0/edi_core_oca/models/edi_exchange_consumer_mixin.py#L449 When we don't have a record, we have to decide how to lookup for edi.confg. Today we always rely on a m2m relation via partner (we have a m2m relation to be able to reuse configs). We should probably state which configs are meant to run globally for a given model (eg: w/ a flag I hope it's clear. Otherwise we can organize a call 😉 |
7a14077 to
08547d9
Compare
|
Hi, @simahawk I've applied the suggested changes. Please let me know if this aligns with what you had in mind. I’ve also included in an extra commit how edi_storage_oca will look with this change to show the full integration (if we merge, this commit will be deleted and added to the other PR). Please let me know if I missed anything or misunderstood something. Thanks! |
| is_global = fields.Boolean( | ||
| string="Global Configuration", | ||
| help="If checked, this configuration will be executed for all records, " | ||
| "regardless of the partner.", |
There was a problem hiding this comment.
| "regardless of the partner.", | |
| "regardless of any related odoo record.", |
| @@ -0,0 +1,24 @@ | |||
| <odoo noupdate="1"> | |||
| <record id="edi_config_trigger_storage_done" model="edi.configuration.trigger"> | |||
| <field name="name">On record exchange done</field> | |||
There was a problem hiding this comment.
aren't these triggers generic enough to stay in the core module? 🤔
| help="Record created from a file found in this FS storage", | ||
| ) | ||
|
|
||
| def _move_file(self, storage, from_dir_str, to_dir_str, filename): |
There was a problem hiding this comment.
I would move this to fs_storage as we are dragging this from project to project, module to module
| functools.partial(move_func, storage, sftp_filepath, sftp_destination_path) | ||
| ) | ||
|
|
||
| def on_edi_exchange_done(self): |
There was a problem hiding this comment.
| def on_edi_exchange_done(self): | |
| def _storage_on_edi_exchange_done(self): |
| res = self._move_file(storage, error_dir, done_dir, file) | ||
| return res | ||
|
|
||
| def on_edi_exchange_error(self): |
There was a problem hiding this comment.
| def on_edi_exchange_error(self): | |
| def _storage_on_edi_exchange_error(self): |
I'm not a fan of adding this custom logic to the record, having it on the component was better.
However, TBH, I have no better place to suggest 🤷
There was a problem hiding this comment.
Yes, I agree with you, I think it's much cleaner to keep using components and move that logic into a glue module, as I did in my first approach.
There was a problem hiding this comment.
we can, but at least we should prefix w/ proper naming (eg: _storage_*).
@etobella your POV?
Move _trigger_edi_event_make_name method to edi_core_oca module in order to make the listener usable without requiring the edi_component_oca module