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.

Sign up for GitHub

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

Jump to bottom

[14.0][MIG] l10n_be_national_number #190

Merged
merged 9 commits into from
Jul 17, 2023
Merged

[14.0][MIG] l10n_be_national_number #190

merged 9 commits into from
Jul 17, 2023

Conversation

carmenbianca
Copy link
Member

@carmenbianca carmenbianca marked this pull request as draft May 23, 2023 14:13
@carmenbianca carmenbianca marked this pull request as ready for review May 23, 2023 14:29
@carmenbianca carmenbianca mentioned this pull request May 24, 2023
Merged
carmenbianca
carmenbianca commented May 24, 2023
View reviewed changes
l10n_be_national_number/models/res_partner.py Outdated
class ResPartner(models.Model):
_inherit = "res.partner"

national_number = fields.Char(string="National Number")
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this field even used? id_numbers appears to be the field that is actually used. This field national_number is displayed in no view. I also can't find downstream usage of this field.

Copy link
Member

Choose a reason for hiding this comment

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

I think you are right ! I think I made a mistake.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's delete this module then ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only important thing in this module is the category definition in the data file :) We can remove this Python code.

victor-champonnois
victor-champonnois approved these changes May 25, 2023
View reviewed changes
Copy link
Member

@victor-champonnois victor-champonnois left a comment

Choose a reason for hiding this comment

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

Wow that's weird, how could it have worked without the import statement !

victor-champonnois and others added 9 commits June 22, 2023 10:21
@victor-champonnois @carmenbianca
[ADD] l10n_be_national_number
14bf2f2 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@carmenbianca
[UPD] Update l10n_be_national_number.pot
a83d04e 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@OCA-git-bot @carmenbianca
[UPD] README.rst
45ee8ec 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@OCA-git-bot @carmenbianca
[ADD] icon.png
a74bf73 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@carmenbianca
[IMP] l10n_be_national_number: black, isort, prettier
1ca1354
@carmenbianca
[MIG] l10n_be_cooperator_national_number: Migration to 13.0
72e86bc 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@carmenbianca
[MIG] l10n_be_cooperator_national_number: Migration to 14.0
74e9ba7 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@carmenbianca
[FIX] l10n_be_national_number: Add forgotten import statement
5541306 
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@carmenbianca
[FIX] l10n_be_national_number: Remove national_number field that was …
a02f5de 
…never used

Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
@carmenbianca carmenbianca force-pushed the 14.0-mig-l10n_be_national_number branch from b1f7ce8 to a02f5de Compare June 22, 2023 08:22
huguesdk
huguesdk reviewed Jun 23, 2023
View reviewed changes
Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

i think the official name for this identification number is “national registration number”. or is this category also used for the bis number? then the module should be called “l10n_be_identification_number” (https://sma-help.bosa.belgium.be/en/faq/i-do-not-know-my-belgian-identification-number).

l10n_be_national_number/data/res_partner_id_category.xml

<record id="l10n_be_national_number_category" model="res.partner.id_category">
<field name="code">l10n_be_national_number</field>
<field name="name">Belgium National Number</field>
Copy link
Member

Choose a reason for hiding this comment

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

shouldn’t this be “Belgian national registration number” (translated to “Numéro de registre national belge”)? or something like “Belgium: national registration number” or ”National registration number (Belgium)”? https://sma-help.bosa.belgium.be/en/faq/where-can-i-find-my-national-registration-number

huguesdk
huguesdk approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@huguesdk huguesdk left a comment

Choose a reason for hiding this comment

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

this is a port of a module in 12.0 that is used in production since several months and that it’s too late too rename things at this stage before deployment, but we should plan a renaming at some point.

otherwise, lgtm!

@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). 🤖

@carmenbianca
Copy link
Member Author

@victor-champonnois can you review/approve this?

remytms
remytms approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@remytms remytms left a comment

Choose a reason for hiding this comment

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

LGTM

@victor-champonnois
Copy link
Member

@carmenbianca Remy just approved so it's good to merge, you don't need my review anymore

@carmenbianca
Copy link
Member Author

@robinkeunen please merge :)

@robinkeunen
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-190-by-robinkeunen-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 19f1439 into OCA:14.0 Jul 17, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 3d36b0d. 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
Reviewers

@huguesdk huguesdk huguesdk approved these changes

@remytms remytms remytms approved these changes

@victor-champonnois victor-champonnois Awaiting requested review from victor-champonnois

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@carmenbianca @OCA-git-bot @victor-champonnois @robinkeunen @huguesdk @remytms