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

[15.0][MIG] hr_timesheet_sheet #516

Merged
merged 167 commits into from
Sep 21, 2022
Merged

[15.0][MIG] hr_timesheet_sheet #516

merged 167 commits into from
Sep 21, 2022

Conversation

CRogos
Copy link
Contributor

@CRogos CRogos commented Aug 23, 2022

Improvement of #484

any idea how to fix these warnings in pre-commit?
hr_timesheet_sheet/i18n/id.po:439: [W7968(po-msgstr-variables), ] Translation string couldn't be parsed correctly using string%variables TypeError('not all arguments converted during string formatting')

MiquelRForgeFlow and others added 30 commits April 22, 2022 13:15
* [10.0] hr_timesheet_sheet

* [11.0][MIG] hr_timesheet_sheet

* [REMOVE] hr_timesheet.sheet.account

* [REMOVE] 'new' state

* [ADD] Tests

* [UPD] Adapt to multicompany

* [ADD] Add more tests (include multicompany tests)

* [FIX] project_task_stage_allow_timesheet: show error message only if task

* [ADD] Migration scripts to v11
Currently translated at 98.9% (88 of 89 strings)

Translation: hr-timesheet-11.0/hr-timesheet-11.0-hr_timesheet_sheet
Translate-URL: https://translation.odoo-community.org/projects/hr-timesheet-11-0/hr-timesheet-11-0-hr_timesheet_sheet/ja/
Currently translated at 100,0% (89 of 89 strings)

Translation: hr-timesheet-11.0/hr-timesheet-11.0-hr_timesheet_sheet
Translate-URL: https://translation.odoo-community.org/projects/hr-timesheet-11-0/hr-timesheet-11-0-hr_timesheet_sheet/pt_BR/
…ay into this module, which adds a configuration to select the week start day.
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
If you run tests on Sunday, test_4 was not prepared for it, as next day is Monday,
which belongs to other week, and thus not included in same timesheet. With this,
we cover that case, decreasing one day instead of adding it.
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
@CRogos
Copy link
Contributor Author

CRogos commented Aug 23, 2022

@dsolanki-initos and @gaikaz could you review again?

@CRogos CRogos marked this pull request as ready for review August 23, 2022 12:20
@CRogos CRogos mentioned this pull request Aug 23, 2022
17 tasks
Copy link
Member

@gaikaz gaikaz left a comment

Choose a reason for hiding this comment

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

Code review: LGTM

Copy link
Contributor

@dsolanki-initos dsolanki-initos left a comment

Choose a reason for hiding this comment

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

Code review functional Test LGTM!
Below are a few points that need to be taken care of

  1. Please use TransactionCase instead of SavepointCase in test-case
  2. Please remove the commented code

@CRogos
Copy link
Contributor Author

CRogos commented Aug 25, 2022

@dsolanki-initos thanks for you review and findings. I've changed to TransactionCase and removed the commented code from the tests.

@CRogos
Copy link
Contributor Author

CRogos commented Sep 16, 2022

@alexey-pelykh do you have an advice how to continue here on the pre-commit issue?

@houssine78
Copy link
Sponsor

hi @pedrobaeza do you have an idea of what could be this pre-commit issue ? thank you

@pedrobaeza
Copy link
Member

Some translations are incorrect:

hr_timesheet_sheet/i18n/id.po:439: [W7968(po-msgstr-variables), ]  Translation string couldn't be parsed correctly using string%variables TypeError('not all arguments converted during string formatting')
(and more)

@CRogos
Copy link
Contributor Author

CRogos commented Sep 19, 2022

They are already wrong at version 14.0 (and earlier)
image

Will all future pre-commit actions fail, after we've merged this? (until the translation of all languages is fixed)
I assume this check is new in pre-commit v15 and maybe should be removed until the translations have been cleaned?

@pedrobaeza
Copy link
Member

No, this check is correct and must be kept. The solution is to fix the translations.

@CRogos
Copy link
Contributor Author

CRogos commented Sep 19, 2022

After or before the PR was merged?
The module only occurs in Weblate, after the PR was merged, correct?
Waiting for all Translations could take infinite time ;) because of to many involved languages and deleting the keys might also not the right solution.

edit:
In some cases might deleting an option? @pedrobaeza you are the Spanish native ;-)
image

@pedrobaeza
Copy link
Member

Youy have to fix it here in the migration.

@CRogos
Copy link
Contributor Author

CRogos commented Sep 19, 2022

hmm.. sorry but I do not speak all these languages. How can I fix this?
I can replace the final "." by ": %s" but I am not sure if this is right in all cases... especially in Japanese, Arabic, etc.
I think this process has to be improved, or I missed some guide on this type of issue.

@pedrobaeza
Copy link
Member

I think the problem is not the language, but incorrect syntax. You just need to fix such syntax. I think it's a missing %s somewhere, but check the indicated line.

@CRogos
Copy link
Contributor Author

CRogos commented Sep 19, 2022

In this case, adding ": %s" might be right in most languages:
image

but this case is already more difficult:
image

@oca-clabot
Copy link

Hey @CRogos, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: https://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@CRogos
Copy link
Contributor Author

CRogos commented Sep 19, 2022

@pedrobaeza Thanks for your input and support.
I did my very best to translate everything and left it in a separate commit on purpose, so that it can easily be backported to v13/14, after it was approved, because the same translation issues are there also.

Is there anything I can do on the failed project test coverage? I mean more than 100% of the patch cannot be tested, correct?
By the way, the link to the CLA is not working, but the cause should be fixed.

@pedrobaeza pedrobaeza added this to the 15.0 milestone Sep 21, 2022
@pedrobaeza
Copy link
Member

/ocabot migration hr_timesheet_sheet
/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-516-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 21216ca into OCA:15.0 Sep 21, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at c8b05d1. Thanks a lot for contributing to OCA. ❤️

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

Successfully merging this pull request may close these issues.