-
-
Notifications
You must be signed in to change notification settings - Fork 856
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
[11.0][MIG] partner_identification #526
Conversation
* [IMP] improve module description * [IMP] Remove useless comments * [FIX] Complete incomplete sentence * [IMP] Replace field 'state' by 'status' in res_partner_id_number * [IMP] Add new field 'Place of Issuance' * [FIX] Readme formatting * [IMP] status is now a selection field * [IMP] use method to provide the default value for validation_code * [IMP] Add help texts * [FIX] Add missing constrains on category_id in res_partner.id_number The number must be validated also when we change the category
Allow for context override of validations using ``id_no_validate``
OCA#419) * [IMP] partner_identification: Add field computation and inverses * Add methods to allow for computation and inverse of an ID field of a specific category type * [IMP] partner_identification: Add search option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nitpicking otherwise LGTM
partner_identification/README.rst
Outdated
@@ -1,3 +1,4 @@ | |||
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg | |||
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have twice the same line
You have twice the same line
@@ -1,17 +1,18 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
header for encoding can be removed utf8 is default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some details.
LGTM
'ACSONE SA/NV,' | ||
'LasLabs,' | ||
'Odoo Community Association (OCA)', | ||
'website': 'https://odoo-community.org/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use the URL of the repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@@ -0,0 +1,7 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coding line is not necessary in v11 (P3)
remove copyright notice from __init__.py
files too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MQT is not ready yet and logs warnings... I was trying to get a clear build. I'll remove them anyway ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But remember that warnings don't change green status.
class TestResPartner(common.SavepointCase): | ||
|
||
@classmethod | ||
def _init_test_model(cls, model_cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simahawk Why do you remove this way to create test models? What's the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon does not work on v11, it breaks transaction. In any case, my replacement sounded like less "black magic" and is working fine in a lot of modules ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simahawk It breaks transaction because one line is missing to disable the commit done by Odoo when a model is created. self.env.cr.commit = mock.MagicMoxk()
or self.env.cr.commit = lambda a: True
With your code, after the tests the partner model is modified into database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean w/ modified? You run this only in test mode, so: db is a test one, everything is rolled back after tests (if you don't commit). Am I missing anything?
Another one with truncated git history due to rename. Could you fix that please? I think a special case for this should be added to https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-11.0 |
I'm not sure if that can be done. It's the only drawback of this method, but I think it's not the most important thing, as renamings should be exceptional cases (and more if everyone takes care of the names on the first review phase). Commit history is still in the corresponding branch. |
🤔 I think that if we care about history, we care about history, and if we dont, we don't. I got the method, it's not that hard: # New temp branch to get the commits until the rename
git checkout -b tmp1 origin/10.0
# Extract oldest commits
module=partner_identifiers
git filter-branch --force --subdirectory-filter $module
# Simplify history by setting all commits directly in the good addon name
module=partner_identification
git filter-branch --force --tree-filter "mkdir -pv $module; git mv -k * $module" HEAD
# New temp branch to get the commits from the rename
git checkout -b 11.0-$module origin/10.0
# Extract commits from rename
git filter-branch --force --subdirectory-filter $module
git filter-branch --force --tree-filter "mkdir -pv $module; git mv -k * $module" HEAD
# Get all commits together
git rebase tmp1
git branch -D tmp1
# Rebase on 11.0 and do manual removal of unneeded commits (translations, addon rename...)
git rebase -i 11.0
# Add your migration commit
git cherry-pick 260925815802f4e7cf1aece2ad7f7a090f695aac With this system, we'll avoid having the problem on 12.0 also because all history will be in |
OK, I haven't dug enough. Let's try the procedure on some of the renamed modules to see if it works. |
I'll give it a try |
@simahawk Can you update commit history and squash your fixup commits ? |
@yajo I'm giving a try to your approach but there are no more commits before these:
and if you look here https://github.com/OCA/partner-contact/commits/10.0?after=f7ec110676a490448baf08c9820cc35a3643c610+34&path%5B%5D=partner_identification is the same. |
I think we can let it as it. This was more a curiosity than a real thing to be taken care of. |
ok, I'll adapt fake models for tests and squash |
a03bc1e
to
a49ae7e
Compare
done. I also replaced svg license image w/ png |
|
||
@api.multi | ||
@api.depends('id_numbers') | ||
def _compute_identification(self, field_name, category_code): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to put api.depends
? Will it work when we use it via lambda?
compute=lambda s: s._compute_identification(
'social_security', 'SSN',
),
squashed and reworded commits "meaningfully"