Skip to content

Conversation

@sergiobstoj
Copy link
Member

Assigns the default partner to the Entries

@sergiobstoj sergiobstoj marked this pull request as ready for review July 21, 2025 08:57
@sergiobstoj
Copy link
Member Author

@OCA/pos-maintainers can this be reviewed?

Copy link

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Hello 👋🏻

It seems this module's unit tests are not being executed in the CI.
It's failing silently with this message:

2025-07-21 08:55:44,408 470 INFO odoo odoo.tests.suite: skipped setUpClass (odoo.addons.pos_default_partner.tests.test_pos_default_partner.TestPosDefaultPartner) : Accounting Tests skipped because the user's company has no chart of accounts. 

What triggered me is the fact that I couldn't find in core the methods you're overriding.

Any chance you can fix that here?


Moreover, from a functional point of view, I don't think I agree with this change.
The pos_default_partner module does exactly what it says: sets a default partner for orders. However, it's only a default value.. other partners can be chosen per different orders.

Then, having this passed onto the single account.move wouldn't be correct IMO.

I would, however, ask for input from the module's author: @bealdav

@sergiobstoj
Copy link
Member Author

sergiobstoj commented Aug 22, 2025

Hi, @ivantodorovich
First of all, thanks for your review.

Having read your comments, ¿it could be an idea to make it configurable what I am proposing in here? ¿Or even including it in a new addon?
Overall I believe it's a good improvement.

About the tests, I added the fix for loading a CoA.
Greetings.

@ivantodorovich
Copy link

ivantodorovich commented Aug 22, 2025

Having read your comments, ¿it could be an idea to make it configurable what I am proposing in here? ¿Or even including it in a new addon?
Overall I believe it's a good improvement.

I think both options are good ideas.
Personally I wouldn't oppose to it being configurable. Module authors' have the last word about such topics, so we'll give them some time to catch up and review.

About the tests, I added the fix for loading a CoA.

Hey, thanks for giving it a try! It seems they're still not being executed, though.

We have an issue with our CI, that gives a false positive ✅ in such cases (tracked here: OCA/oca-ci#105)

🧠 How to tell if they're being executed, then?

1️⃣ Well, the first and most obvious one is the coverage report:

2️⃣ Then, there's also this log line in the build logs:

2025-08-22 05:43:43,275 470 INFO odoo odoo.tests.suite: skipped setUpClass (odoo.addons.pos_default_partner.tests.test_pos_default_partner.TestPosDefaultPartner) : Accounting Tests skipped because the user's company has no chart of accounts. 

@bealdav
Copy link
Member

bealdav commented Sep 3, 2025

Hi @ivantodorovich thanks to ping me.

FYI information I didn't use this module. I prefer to be away from POS projects.
Particularly point_of_sale module which duplicate code, and workflows to make simple things complex and confusing for users i.e. pos.order vs sale.order and then all implications with update/cancellation. In 95% of the use of pos in real life, you don't need to be disconnected, then why this complexity ?

Then I'll let main contributors of this module replace me as author.

I'm sure your options will be more consistent than mine.

Thanks a lot.

@danielduqma danielduqma force-pushed the 16.0-imp-pos_default_partner-moves_default_partner branch from 325d1d4 to d061de8 Compare December 26, 2025 06:20
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.

4 participants