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

[10.0] IMP l10n_it_rea #612

Merged
merged 1 commit into from
Oct 18, 2018
Merged

[10.0] IMP l10n_it_rea #612

merged 1 commit into from
Oct 18, 2018

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Oct 16, 2018

FIX REA fields order
ADD REA fields to copany and compute company_registry accordingly

@eLBati eLBati mentioned this pull request Oct 16, 2018
@eLBati eLBati added this to the 10.0 milestone Oct 16, 2018
Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
Code and runbot review

Please update the README with your changes too.
Also, could you fix the broken link in the README too?

@@ -1,42 +1,52 @@
# Translation of Odoo Server.
Copy link
Member

Choose a reason for hiding this comment

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

Do not edit po* files

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we already merged several PRs directly changing po files and no issues were raised. Also, in this case, I checked that no changes were committed by weblate to l10n_it_rea/i18n/it.po
In case of conflicts, I will fix them.

Finally, as previously said, I need to add Italian translation to the module and I can't overload my development workflow

Copy link
Member

Choose a reason for hiding this comment

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

Then we are lucky that no-one is translating those modules :)
Not editing po* files is safer as per https://docs.google.com/document/d/1b7lzeKyaDJ_zgmZ3VKWNqJyFb5gnsdXF41-DtPKLvRY/edit#heading=h.rk8pleq1juou, but you can freely edit them, just be sure to take care of possible conflicts later.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact you should never push change to .po files.

If you already have a .po file, upload it to weblate.

Here https://docs.google.com/document/d/1b7lzeKyaDJ_zgmZ3VKWNqJyFb5gnsdXF41-DtPKLvRY/edit# i see that

.pot files are regenerated by Travis and pushed to GitHub as soon as a change is pushed to a main branch in OCA.

...

Weblate pulls .pot updates from GitHub hourly (at hh:15).
This can be made instantaneous if you add a push event webhook to your project (https://translation.odoo-community.org/hooks/github)

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we are lucky that no-one is translating those modules :)

Actually @primes2h is.

@SimoRubi @primes2h see a07d9c7#diff-bfe1ddd49978694780f98638bfb14cfaR51
and
167f054#diff-bfe1ddd49978694780f98638bfb14cfaR53

I changed l10n_it_fatturapa_out/i18n/it.po in commit a07d9c7 , then @primes2h translated that term , using weblate, in commit 167f054
No issues

If you already have a .po file, upload it to weblate

I could follow this way if terms were already present in stable branches, otherwise I should anyway wait for the PR to be merged, then upload .po file to weblate (or translate by web)

Copy link
Contributor

@primes2h primes2h Oct 17, 2018

Choose a reason for hiding this comment

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

Ok, let's try removing it.po from the PR. @primes2h consider that your work, as Italian translation master, could increase, as I expect few people will translate using weblate after merge sweat_smile

I'll do my best to increase that number, they are so few... :-p

Seriously, this means less work for you before making PR, you can use saved time to help translating/reviewing in weblate later. ;-)
Moreover, I think it can be useful when someone want to test new features in UI.
He can get immediately which and where new added terms are (if other strings have already been translated obviously).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@SimoRubi @primes2h OCA/commission#151 (comment) no_mouth

Per me nessun problema se c'è chiarezza su chi poi si prende in carico la traduzione del modulo, deve essere lo stesso che modifica direttamente i .po tramite la PR.

Sono da evitare le situazioni in cui c'è una persona che traduce/revisiona su weblate e perde tutto il lavoro a causa del merge di una PR fatta da un altro.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@eLBati When you push changes to .po files, you risk git merge conflicts with pushes coming from weblate. It will not happen always of course, if nobody is translating at the same time, and there are no .pot file changes, etc. So it's delicate and people really need to understand in detail what they are doing.

When such a merge conflict happens, it's really messy to resolve because you (well, rather, me) currently need shell access to the weblate server to resolve it (unless you accept losing translations). So yeah, it's messy.

That's why I insist .po files are the responsibility of weblate only.

@eLBati @sbidoul
I think there could be another potential issue in pushing changes directly to .po files.
I just saw that some it.po files of l10n_italy 12.0 repository modules have the wrong version.
"Project-Id-Version: Odoo Server 10.0\n"
but related .pot file has 12.0.

This is probably caused by pushing changes to it.po manually, forgetting to update that line
(In the actual workflow It should be made automatically by weblate, but it doesn't happen if you push changes to .po).

I don't know if this cause issues in the flow, but it could.

@@ -0,0 +1,55 @@
# -*- coding: utf-8 -*-
# Copyright 2018 Lorenzo Battistini

Copy link
Member

Choose a reason for hiding this comment

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

'rea_liquidation_state'
]._description_selection(self.env)
)[company.rea_liquidation_state]
company_registry = _("%s %s Share Cap. %s%s %s %s") % (
Copy link
Member

Choose a reason for hiding this comment

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

Check currency.position to see if the symbol goes before or after the amount, or try to take advantage of a Monetary field

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimoRubi do you know in this case how could I take advantage of a Monetary field?

Copy link
Member

Choose a reason for hiding this comment

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

That would be more nicely displayed in the company form, actually I don't know how that could be useful in string substitution

@eLBati
Copy link
Member Author

eLBati commented Oct 16, 2018

@SimoRubi thanks, I made the other changes

@eLBati eLBati force-pushed the 10.0-fix-rea-fields branch 2 times, most recently from 82a51a2 to ce34a2f Compare October 18, 2018 07:42
@eLBati
Copy link
Member Author

eLBati commented Oct 18, 2018

@primes2h following #611 (comment), I moved REA fields to another tab in company form.
I would keep module's scope and name unchanged for now.

@SimoRubi @primes2h do you think this can be merged?

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Requests from previous reviews are not blocking, just consider #612 (comment): it is ok if you do not want to use Monetary field, but check the currency's position

@eLBati eLBati force-pushed the 10.0-fix-rea-fields branch 2 times, most recently from 5ffd9ac to e61f3b0 Compare October 18, 2018 08:04
@eLBati
Copy link
Member Author

eLBati commented Oct 18, 2018

@SimoRubi I would keep rea_capital a Float because there are other modules using it (as related). I fixed the currency symbol to €, because it is an Italian register. I also refactored the computation of company_registry, using onchange, because in a multinational environment user would need to set company_registry manually

@primes2h
Copy link
Contributor

primes2h commented Oct 18, 2018

@primes2h following #611 (comment), I moved REA fields to another tab in company form.

Much better. 👍

I would keep module's scope and name unchanged for now.

Ok.

@SimoRubi @primes2h do you think this can be merged?

Please have a look at #611:

__manifest__.py file
I changed module name to have a uniform naming convention, I saw other modules already use it.
In website field I put a link to github version/module, you point to github repository, what is better?

DESCRIPTION.rst
I made tiny changes in the original text, I also added an Italian description.

If you agree with these changes (mixing with yours if you want) you could include them in v.10.0 too.

'rea_liquidation_state'
]._description_selection(self.env)
)[self.rea_liquidation_state]
# using always €, as
Copy link
Member

Choose a reason for hiding this comment

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

as...?

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimoRubi fixed

@eLBati
Copy link
Member Author

eLBati commented Oct 18, 2018

@primes2h

I changed module name to have a uniform naming convention, I saw other modules already use it

Changed

In website field I put a link to github version/module, you point to github repository, what is better?

It is up to you, both are valid: https://github.com/OCA/maintainer-tools/blob/master/template/module/__manifest__.py#L11

If you agree with these changes (mixing with yours if you want) you could include them in v.10.0 too

Done.

Thanks

@primes2h
Copy link
Contributor

In website field I put a link to github version/module, you point to github repository, what is better?

It is up to you, both are valid: https://github.com/OCA/maintainer-tools/blob/master/template/module/__manifest__.py#L11

Ok, I think it would be better to use link of the version/addon.

ADD REA fields to copany and compute company_registry accordingly
@eLBati
Copy link
Member Author

eLBati commented Oct 18, 2018

Ok, I think it would be better to use link of the version/addon

@primes2h done

@eLBati eLBati merged commit d54a68c into OCA:10.0 Oct 18, 2018
primes2h pushed a commit to primes2h/l10n-italy that referenced this pull request Oct 18, 2018
ADD REA fields to copany and compute company_registry accordingly
eLBati added a commit to eLBati/l10n-italy that referenced this pull request Oct 31, 2018
ADD REA fields to copany and compute company_registry accordingly
SimoRubi pushed a commit to SimoRubi/l10n-italy that referenced this pull request Aug 7, 2019
ADD REA fields to copany and compute company_registry accordingly
SimoRubi pushed a commit to SimoRubi/l10n-italy that referenced this pull request Aug 7, 2019
ADD REA fields to copany and compute company_registry accordingly
jado95 pushed a commit to jado95/l10n-italy that referenced this pull request Oct 30, 2020
ADD REA fields to copany and compute company_registry accordingly
Borruso pushed a commit to Borruso/l10n-italy that referenced this pull request Apr 9, 2021
ADD REA fields to copany and compute company_registry accordingly
TheMule71 pushed a commit to TheMule71/l10n-italy that referenced this pull request Nov 2, 2022
ADD REA fields to copany and compute company_registry accordingly
TheMule71 pushed a commit to TheMule71/l10n-italy that referenced this pull request Nov 2, 2022
ADD REA fields to copany and compute company_registry accordingly
TheMule71 pushed a commit to TheMule71/l10n-italy that referenced this pull request Nov 2, 2022
ADD REA fields to copany and compute company_registry accordingly
TheMule71 pushed a commit to TheMule71/l10n-italy that referenced this pull request Nov 2, 2022
ADD REA fields to copany and compute company_registry accordingly
SirTakobi pushed a commit to SirTakobi/l10n-italy that referenced this pull request Nov 10, 2022
ADD REA fields to copany and compute company_registry accordingly
SirTakobi pushed a commit to SirTakobi/l10n-italy that referenced this pull request Nov 10, 2022
ADD REA fields to copany and compute company_registry accordingly
TheMule71 pushed a commit to TheMule71/l10n-italy that referenced this pull request Nov 10, 2022
ADD REA fields to copany and compute company_registry accordingly
TheMule71 pushed a commit to TheMule71/l10n-italy that referenced this pull request Nov 25, 2022
ADD REA fields to copany and compute company_registry accordingly
TonyMasciI pushed a commit to saydigital/l10n-italy that referenced this pull request Jan 9, 2023
ADD REA fields to copany and compute company_registry accordingly
TonyMasciI pushed a commit to saydigital/l10n-italy that referenced this pull request Jan 16, 2023
ADD REA fields to copany and compute company_registry accordingly
TonyMasciI pushed a commit to saydigital/l10n-italy that referenced this pull request Jan 27, 2023
ADD REA fields to copany and compute company_registry accordingly
are-agilebg pushed a commit to are-agilebg/l10n-italy that referenced this pull request May 10, 2023
ADD REA fields to copany and compute company_registry accordingly
are-agilebg pushed a commit to are-agilebg/l10n-italy that referenced this pull request May 10, 2023
ADD REA fields to copany and compute company_registry accordingly
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.

4 participants