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

[16.0] [MIG] l10n_ch_mis_reports: Migration to 16.0 #709

Merged
merged 18 commits into from
Jan 18, 2024

Conversation

jguenat
Copy link
Member

@jguenat jguenat commented Aug 8, 2023

Migration with following refactor

  • Use formulas based on account code instead of account tag, l10n_ch_account_tags not required anymore. It was not very practical to have to assign a tag to each account.
  • move data to xml noupdate records : you can modify your templates/styles without loosing the modifications on update.
  • simplify styles

reviews/opinions are welcome :)

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 10, 2023
Copy link

@ecino ecino left a comment

Choose a reason for hiding this comment

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

I have no experience with this module in the previous versions, but it looks good to me (only code review). One quetion, shouldn't we put the XML files in English and provide translations instead of directly entering records in French?

@jguenat
Copy link
Member Author

jguenat commented Dec 12, 2023

I have no experience with this module in the previous versions, but it looks good to me (only code review). One quetion, shouldn't we put the XML files in English and provide translations instead of directly entering records in French?

Thanks for your review !

Yes it would be better but I don't know the english translation. As this is specific for switzerland I guess it's not a big deal if we put it in french, people can then translate it with weblate if needed.

@ecino ecino removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 14, 2023
<field
name="expression"
>-balp[('code', '>=', '3'),('code', '&lt;', '39')][]</field>
<field name="style_id" ref="style_details" />

Choose a reason for hiding this comment

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

Thanks for your work.
Could you juste change the style here with

Suggested change
<field name="style_id" ref="style_details" />
<field name="style_id" ref="style_header" />

Otherwise it's all ok for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your review. Indeed it's nicer with the header style.

@jguenat jguenat force-pushed the 16.0-mig-l10n_ch_mis_reports branch from 928aae9 to dd34f2c Compare January 16, 2024 17:41
Copy link

@eicher31 eicher31 left a comment

Choose a reason for hiding this comment

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

Ok for me

@eicher31
Copy link

@ecino could you merge it? Thanks

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA OCA deleted a comment from OCA-git-bot Jan 18, 2024
@ecino
Copy link

ecino commented Jan 18, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-709-by-ecino-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e208e8f into OCA:16.0 Jan 18, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 1600fc0. 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.

8 participants