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

17.0 mig purchase advance payment #6

Merged
merged 12 commits into from
Apr 8, 2024

Conversation

maciej-wichowski
Copy link

No description provided.

@@ -449,4 +448,4 @@ <h2><a class="toc-backref" href="#toc-entry-7">Maintainers</a></h2>
</div>
</div>
</body>
</html>
</html>

Choose a reason for hiding this comment

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

Looks like you are removing newline. Should fix your editor to not mess up file endings:)

Copy link
Author

Choose a reason for hiding this comment

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

This was pre-commit fix I think.

Choose a reason for hiding this comment

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

Hm, if its done by pre-commit, it looks like its doing something wrong. Usually it should do the opposite, cause now github is throwing "warning":)

Copy link
Author

Choose a reason for hiding this comment

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

Yep, looks like it was editor not pre-commit after all...

"rate": 1.20,
"currency_id": cls.currency_usd.id,
"inverse_company_rate": 1.20,
"currency_id": cls.currency_euro.id,

Choose a reason for hiding this comment

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

Is this really gonna help? Thing with tests that involve company currency or related is tricky for main company. Cause some modules can replace the currency, but if some other modules are installed it can change it to another. So you end up with either USD or EUR currency (but it is not consistent). This can fail if other case happens. Unless you are sure here we will always have EUR?

Copy link
Author

Choose a reason for hiding this comment

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

Well, all previous tests are made assuming, that USD is base currency. My only change was made to fix the failing tests so that rate is created for EUR currency and not USD. Which did not make any sense, honestly I don't know why it worked prior to v17.

Choose a reason for hiding this comment

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

As I said, depending what module is installed, it can have different currency. So there are cases when company currency is different and then tests become inconsistent. For example tests when different COA (localization) is installed:). Better approach would be to test on fresh company, where we would not need to rely on set data that cant even be changed. Though for now, lets hope this won't blow up randomly, which is the most annoying failure:)

Copy link
Author

Choose a reason for hiding this comment

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

I agree. Although since this is an OCA module, I'm surprised it didn't blow up in previous versions...

@maciej-wichowski maciej-wichowski force-pushed the 17.0-mig-purchase_advance_payment branch from aa29ba8 to 93cf953 Compare April 8, 2024 11:44
@maciej-wichowski maciej-wichowski merged commit 074556e into 17.0-versada Apr 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

10 participants