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

[ADD] [13.0] account_product_move enhanced #1544

Open
wants to merge 20 commits into
base: 13.0
Choose a base branch
from

Conversation

NL66278
Copy link
Contributor

@NL66278 NL66278 commented Sep 6, 2023

This module is already used successfully in a live environment at one of our customers for quite some time. However, the PR offering the module was actually never merged. Now the customer needs have expanded, and again we think more organizations could benefit from this.

@NL66278
Copy link
Contributor Author

NL66278 commented Sep 6, 2023

Builds on: #1535

@NL66278 NL66278 force-pushed the 13.0-account_product_move_enhanced branch 21 times, most recently from 7997a21 to 7038de6 Compare September 8, 2023 18:17
@NL66278
Copy link
Contributor Author

NL66278 commented Sep 8, 2023

Can anyone explain to me while with all files 100% covered by tests, the codecov/project still complains about decreased coverage?

@NL66278
Copy link
Contributor Author

NL66278 commented Sep 8, 2023

@ntsirintanis @lfreeke Please review.

@simahawk simahawk changed the title [ADD] [13.0] account product move enhanced [ADD] [13.0] account_product_move enhanced Sep 11, 2023
@simahawk
Copy link
Contributor

Can anyone explain to me while with all files 100% covered by tests, the codecov/project still complains about decreased coverage?

That's normal for new modules. Just look at the diff stats in such cases 😉

@simahawk
Copy link
Contributor

Could you please add a bit of description here? By reading the title I think of a new module due to "ADD" but "enhanced" makes me think about an improving of something existing... and when I look at the version, it says "...4.0.0" which means it's something that was already there.... but where? Or.. the version is wrong!

And what about #1535? Has to be closed? 😵 😄

@NL66278
Copy link
Contributor Author

NL66278 commented Sep 11, 2023

@simahawk I expanded the description. Basically this module is already in use at one of our customers for some time, That's why the version numbers, supporting migration of older data. Other organizations might also be using the module already from the PR. As for the normal red cross for coverage for new modules: the problem is lots of reviewers will not even look at PR's that do not pass all tests. let alone merge them. So if something could be done about that, it would be great, no only for this module, but for all.

@simahawk
Copy link
Contributor

@NL66278 tnx for the update.

Other organizations might also be using the module already from the PR.

Fine, just explaining this in the description is enough for the rest of the world to understand and review based on this 😉

As for the normal red cross for coverage for new modules: the problem is lots of reviewers will not even look at PR's that do not pass all tests.

That's not true, at least from my experience. In fact, not everybody care - or care enough - about test cov not to mention full test cov. It's unlikely that someone will block such PRs just because there's a not perfect score on codecov validation 😉

In any case, thanks for providing full test cov: we need more PRs with this approach :)

ntsirintanis and others added 20 commits September 11, 2023 10:15
It is now possible to filter the appropriate Account Product Move record
by defining a filter on account.move and selecting that. In this way
different move templates can be linked to an account.move, depending
on the applied domain.
There is also a change where debit and credit in extra move definition are
always in the currency of the specific account.product.move.line.
@NL66278 NL66278 force-pushed the 13.0-account_product_move_enhanced branch from 7038de6 to e1dc1b9 Compare September 11, 2023 08:15
@NL66278
Copy link
Contributor Author

NL66278 commented Sep 11, 2023

After a suggestion from @yajo I rebased the branch and all coverage is green now. Apparently the project coverage compares the submitted branch with the current (13.0) branch, instead of the current branch as it is now, with how it would change after merging the PR. Anyway I have now a way to fix these problems. Thanks @yajo and @simahawk for your comments!

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.

None yet

3 participants