Skip to content

[19.0][IMP] edi_*_oca: skip exchange types w/o handler in cron domains#273

Open
HviorForgeFlow wants to merge 4 commits into
OCA:19.0from
ForgeFlow:19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons
Open

[19.0][IMP] edi_*_oca: skip exchange types w/o handler in cron domains#273
HviorForgeFlow wants to merge 4 commits into
OCA:19.0from
ForgeFlow:19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons

Conversation

@HviorForgeFlow

@HviorForgeFlow HviorForgeFlow commented May 12, 2026

Copy link
Copy Markdown
Member

Noticed while developing that when a exchange type is not linked to a handler and the EDINotImplementedError is raised, a DB rollback takes places undoing all previous work.

To ensure that skipping not handled exchanges does not break anything outside the cron scope I had to slightly change the exchange_generate_send method.

CC @etobella @simahawk @ForgeFlow

@OCA-git-bot

OCA-git-bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

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

@OCA-git-bot OCA-git-bot added mod:edi_core_oca Module edi_core_oca series:19.0 mod:edi_queue_oca Module edi_queue_oca labels May 12, 2026
@simahawk

simahawk commented May 14, 2026

Copy link
Copy Markdown
Contributor

Wondering: why is it possible to handle an exc type w/o handler? It should be validated beforehand...

We should make specific validators required based on the direction.
And maybe improve _$direction_new_records_domain.

@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch from 11c86d7 to 960d553 Compare May 16, 2026 07:59
@OCA-git-bot OCA-git-bot added the mod:edi_purchase_oca Module edi_purchase_oca label May 16, 2026
@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch 2 times, most recently from 8d1870f to c20e67c Compare May 16, 2026 08:04
@OCA-git-bot OCA-git-bot removed mod:edi_purchase_oca Module edi_purchase_oca mod:edi_queue_oca Module edi_queue_oca labels May 16, 2026
@HviorForgeFlow

HviorForgeFlow commented May 16, 2026

Copy link
Copy Markdown
Member Author

Wondering: why is it possible to handle an exc type w/o handler? It should be validated beforehand...

We should make specific validators required based on the direction. And maybe improve _$direction_new_records_domain.

That makes more sense, cleaner implementation, thanks for the advice

@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch from c20e67c to bcb249e Compare May 18, 2026 06:56
@OCA-git-bot OCA-git-bot added the mod:edi_queue_oca Module edi_queue_oca label May 18, 2026
@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch 3 times, most recently from 4db716b to 2134f83 Compare May 18, 2026 07:21
@HviorForgeFlow HviorForgeFlow changed the title [19.0][IMP] edi_core_oca: gracefully handle EDINotImplementedError to prevent batch failures on crons [19.0][IMP] edi_core_oca: skip exchange types w/o handler in cron domains May 18, 2026
@HviorForgeFlow

HviorForgeFlow commented May 18, 2026

Copy link
Copy Markdown
Member Author

@simahawk it follows your suggested approach

@OCA-git-bot

OCA-git-bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@simahawk

simahawk commented May 18, 2026

Copy link
Copy Markdown
Contributor

@HviorForgeFlow I think you miss the part "specific validators required based on the direction."
Now that I see the change I'm not really a big fan of just the domain fix. Sorry 😇

What about adding validation and fixing the data the you have?
I mean, an exc type that has direction "output" and has no "generate" or "send" handler is definitely broken.
I'd rather:

  • add validation on the exc type form (make some fields required based on direction)
  • add an api.constraint

WDYT?

@simahawk

simahawk commented May 27, 2026

Copy link
Copy Markdown
Contributor

@HviorForgeFlow ping :)

@HviorForgeFlow

HviorForgeFlow commented May 27, 2026

Copy link
Copy Markdown
Member Author

Proposal on adding validators and constraints to the exchange type as you were suggesting @simahawk

@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch from f9cdf6b to 7eb70b3 Compare May 27, 2026 12:14
@OCA-git-bot OCA-git-bot added the mod:edi_endpoint_oca Module edi_endpoint_oca label May 27, 2026
@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch 2 times, most recently from 6e132ca to 63cfd44 Compare May 27, 2026 12:25
@HviorForgeFlow HviorForgeFlow changed the title [19.0][IMP] edi_core_oca: skip exchange types w/o handler in cron domains [19.0][IMP] edi_*_oca: skip exchange types w/o handler in cron domains May 27, 2026
@HviorForgeFlow
HviorForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch from c23fb91 to a4b5e52 Compare July 17, 2026 10:46
@OCA-git-bot OCA-git-bot added mod:edi_purchase_oca Module edi_purchase_oca mod:edi_sale_oca Module edi_sale_oca labels Jul 17, 2026
@MiquelRForgeFlow
[FIX] edi: add no-op handler to satisfy handler constraint
8d889b1 
The new _check_direction_handlers constraint broke installation with
demo data and several test suites. Add edi.oca.handler.noop in
edi_core_oca, reference it from the affected demo exchange types, and
make test exchange types default to it when no handler is registered.
@MiquelRForgeFlow
MiquelRForgeFlow force-pushed the 19.0-imp-edi_core_oca-skip-not-edi-implement-on-crons branch from e9a6fd8 to 8d889b1 Compare July 17, 2026 11:11
@MiquelRForgeFlow

MiquelRForgeFlow commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@simahawk could you check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@AaronHForgeFlow AaronHForgeFlow AaronHForgeFlow approved these changes
@MiquelRForgeFlow MiquelRForgeFlow MiquelRForgeFlow approved these changes
@JordiMForgeFlow JordiMForgeFlow JordiMForgeFlow approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved mod:edi_core_oca Module edi_core_oca mod:edi_endpoint_oca Module edi_endpoint_oca mod:edi_purchase_oca Module edi_purchase_oca mod:edi_queue_oca Module edi_queue_oca mod:edi_sale_oca Module edi_sale_oca ready to merge series:19.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@HviorForgeFlow @OCA-git-bot @simahawk @MiquelRForgeFlow @AaronHForgeFlow @JordiMForgeFlow