[13.0] [MIG] partner_multi_relation #936

NL66278 · 2020-06-29T22:38:19Z

My attempt at migration of the module. Including all the fixes already present in 12.0.

pedrobaeza · 2020-06-30T07:02:55Z

@NL66278 I see you are not following the migration guide:

https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-13.0

for proper commit separation for things like auto-formatting. Can you please check?

NL66278 · 2020-06-30T07:49:06Z

@pedrobaeza I agree that I could have split commit 221c324 to have the header correction on test and the other changes in test in separate commits, but I think in the other commits I did not mix up functional changes and changes for conventions. Please take into account that I build on the migration work don by @Rad0van in commit fa907bb, and then cherry-picked the commits from the 12.0 branch that were missing.

pedrobaeza · 2020-06-30T07:52:39Z

I mean to split the [IMP] $MODULE: black, isort, prettier commit from the migration one, being the auto-formatting previous to that one, and avoid later commits like 8f4ff0a. With current approach, each time you touch the code for fixing something, you will mix formatting tasks with code patches.

NL66278 · 2020-06-30T08:30:02Z

@pedrobaeza But I have separate commits for black, isort etc, and on the other hand for functional changes. So what exactly do you want different?

pedrobaeza · 2020-06-30T09:26:48Z

The problem is the order in which you have made such operations: first should be auto-formatting for all the existing code. Then you add the rest of the code (that is auto-formatted by default). If not done that way, as said, you mix diffs with code formatting and patches.

pedrobaeza · 2020-06-30T09:27:53Z

Anyway, I'm missing a lot of commit history from previous branches...

I think you need to redo from scratch the migration, or at least redo the procedure and then mount on top the 13.0 stuff from you and the other contributor.

* API of _auto_init * Menu and groups stuff has moved from base to sales_team in odoo 10

* Fix error on search when leaf is (1, '=', 1) * Another comparison fix with search arguments

…f relation.

* Update README * Python 2to3 * Move relation menus items under contacts * Fix date comparison * Fix cache invalidate on relation write * Bump module version * Remove test_relation_type_unlink_dberror This test is not required because it tests a standard behavior of SQL. If a foreign key column is not nullable, then the foreign row can not be deleted. * Remove utf8 encoding comment * Add missing dependency for partner_multi_relation * Add missing active_test in view action

* Bug Fix * Enable additional columns on res_partner_relation_all. Enable to add additional columns from res_partner_relation in the sql view of res_partner_relation_all. * Only require inverse_name when relation type is not symmetric * Remove deprecated @api.one and replace openerp with odoo * Add website to manifest * Prevent disabling allow_self if reflexive relations exist * Enable deleting/ending invalid relations after deactivating reflexivity. * Handle case of invalid future reflexive relations

Updated by Update PO files to match POT (msgmerge) hook in Weblate.

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: partner-contact-12.0/partner-contact-12.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-12-0/partner-contact-12-0-partner_multi_relation/

Currently translated at 86.6% (84 of 97 strings) Translation: partner-contact-12.0/partner-contact-12.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-12-0/partner-contact-12-0-partner_multi_relation/es/

[IMP] Flake8

HaraldPanten

Hi @NL66278 here's my functional review:

I tried this use case:

Company A and Company B have a "is competitor of" relation between themselves. Both are companies.
"is competitor of" relation type has "Do not allow change that will result in invalid relations" invalid relation handling configuration.

TEST A:

1- Open Company A and check partner relations.
2- Navigate to partner form (from partner relations tree view) and change "Company" to "Individual".
3- An error is shown, but the change is done despite of the restriction.

TEST B:

1- Open company A form view and change "Company" to "Individual".
2- The change is done despite of the "is competitor of"//"Do not allow change that will result in invalid relations" restriction. Even the error message is not shown if I make this change directly from the partner form (instead of the partner relations tree view as I mentioned in TEST A).

IMO this fix should be done.

My IMP recommendation would be to show the restriction/field details in the error pop-up if user is trying to make an invalid configuration. If a contact has lots of different relationship types with other contacts, that would help a lot to find where are the restrictions, so... it would improve UX.

Imagine the TEST A and B defined before. That would be an example of the IMP that I'm proposing:

Another use case/issue is that "invalid relation handling" seems not to be working properly, for example:

Company A and Company B have a "is competitor of" relation between themselves. Both are companies.
"is competitor of" relation type has "Do not allow changes that will result in invalid relations" invalid relation handling configuration.

TEST C:

1- I open Relation Types and I have "Do not allow changes that will result in invalid relations" in "is competitor of" invalid relation handling.

2- I change "Do not allow changes that will result in invalid relations" parameter by "Allow existing relations that do not fit changed conditions" invalid relation handling.

3- I select a partner and I check it's relations.

4- From the relations tree view, I navigate directly to the Company A and I change it's type to "Individual".

Issue: This error message is shown but IMO there shouldn't be any restrictions as I configured "is competitor of" invalid relation handling as "Allow existing relations that do not fit changed conditions".

Same happens when I select "End relations per today if they do not fit changed conditions" or "Delete relations that do not fit changed conditions".

As a UX IMP, I propose you to set different messages depending on the invalid relation handling action. Imagine that you're going to delete relations because you've configured "Delete relations that do not fit changed conditions" and you're going to omit a restriction. Maybe a warning like --> You're going to delete this contact relations because you're not respecting the 'Delete relations that do not fit changed conditions' invalid relation handling.

What do you think? Hope it helps you!

NL66278 · 2020-07-09T14:16:27Z

@HaraldPanten Thank you for your review. Note that this is a migration PR, but your tests exposed some shortcomings in the existing code, and some limitations that perhaps have not been properly pointed out.

In Test A and in Test B you are changing the partner. At present the test and the settings for how to handle existing relations are a constraint on the relation type or the relation itself. Not on the partner. Therefore the change on the partner is allowed. I agree with you the code could or rather should be enhanced to prevent changes on the partner that would result in invalid relations.

This also explains why you sometimes get a message for changing a partner, but only when you change it from the relations view.

In test C the error is correct - we might discuss the message -, as you are changing an existing relation by changing the attributes of the partner. "Allow existing relations...." basically means that if you change the conditions for a relation, existing relations are not deleted or ended.

I will look into improving the error messages, and extending the checks to changes from the partner side.

It is not easy with Odoo to intercept a change that might have unwanted consequences - like deleting existing relations when the relation type is changed, than show a message, but allow the user to override the message and proceed...

BT-jstein · 2020-08-20T15:28:39Z

@NL66278 Thanks for your effort! We would highly appreciate to see the pull request merged since we are running a V13 project where this module covers the requested functionality.

pedrobaeza · 2020-09-07T07:26:10Z

@BT-jstein if you want to see it merged, contribute with your review to this PR: https://odoo-community.org/page/review

rousseldenis

Code review

regispirard

Functional review : LGTM.

Tested :

A) Configuration of use of relation type :

With left type person / right type organisation : ok
Reflexive : ok
Symmetric : ok
with right partner category (label) : ok

B) Invalid relation handling:

do not allow : ok
allow : ok
end relation : ok
delete : ok

C) Search in Relations / Relations (res.partner.relation.all) : Ok
Just noticed there was a filter "Include past records" which seems to have no use since the tree view already show all records.
I guess it was originally designed to show only active records, and this filter allowed to show active + closed records.
Anyway, it was also the case in V12 so, this might be an improvement and should not block migration.

Thank you

pedrobaeza · 2020-09-07T08:40:04Z

/ocabot merge nobump

OCA-git-bot · 2020-09-07T08:40:47Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-936-by-pedrobaeza-bump-nobump, awaiting test results.

Signed-off-by pedrobaeza

OCA-git-bot · 2020-09-07T08:52:58Z

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 13.0-ocabot-merge-pr-936-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

rousseldenis · 2020-09-07T09:11:39Z

@pedrobaeza It seems that http://download.geonames.org/export/zip/MC.zip is failing again... So, merging is blocked until they sove it...

pedrobaeza · 2020-09-07T09:13:15Z

Yes, let's wait a bit. It's a temporary glitch as you know.

rousseldenis · 2020-09-07T09:31:58Z

Yes, let's wait a bit. It's a temporary glitch as you know.

IMHO, that module should be moved to its own repo in order to not bother anymore people work.

pedrobaeza · 2020-09-07T09:34:55Z

I don't think the same, as this only happens once in a couple of months. I'll be monitoring this. Don't worry.

rousseldenis · 2020-09-07T09:48:32Z

I don't think the same, as this only happens once in a couple of months. I'll be monitoring this. Don't worry.

The problem is that you are not the only one working here.

pedrobaeza · 2020-09-07T09:52:28Z

Nor you, and as said, I'm taking care of this. If I am the one that has started the merge process, why are you ranting about something that doesn't affect you directly? If you have one PR affected, then say it and we find a solution. Meanwhile, please don't start again with the same thing.

rousseldenis · 2020-09-07T10:11:46Z

Nor you, and as said, I'm taking care of this. If I am the one that has started the merge process, why are you ranting about something that doesn't affect you directly? If you have one PR affected, then say it and we find a solution. Meanwhile, please don't start again with the same thing.

I try just to find a more long-term solution as people should have an environment as stable as possible and not influenced by external factors that don't depend on their code. That can be frustrating when contributing. That's all I say.

pedrobaeza · 2020-09-07T11:47:44Z

This is working since v8 (6 years), and it's not a great problem. Only very rare occasions.

pedrobaeza · 2020-09-08T06:43:27Z

/ocabot merge nobump

OCA-git-bot · 2020-09-08T06:43:42Z

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

OCA-git-bot · 2020-09-08T07:18:37Z

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

NL66278 mentioned this pull request Jun 30, 2020

[13.0] [MIG] partner_multi_relation #913

Closed

NL66278 and others added 23 commits July 6, 2020 19:44

[MIG] partner_multi_relation: Migration to 10.0

1f4420f

[FIX] partner_multi_relation: Several fixes:

1e248ae

* API of _auto_init * Menu and groups stuff has moved from base to sales_team in odoo 10

[IMP] Menu order and v10 deprecations

628cbf4

OCA Transbot updated translations from Transifex

ebd2b4b

[FIX] partner_multi_relation: Fix errors on searches

477c7b0

* Fix error on search when leaf is (1, '=', 1) * Another comparison fix with search arguments

OCA Transbot updated translations from Transifex

4ff5360

[IMP] Make partner_multi_relation more extendable.

e3c67b3

[FIX] partner_multi_relation. Drop is_inverse from vals on creation o…

d71d2aa

…f relation.

[FIX] partner_multi_relation. Wrong self instead of this.

ae7c15a

[FIX] partner_multi_relation. Extendable but threadsafe.

50a7e55

[UPD] Update partner_multi_relation.pot

4599c13

Migrate partner_multi_relation to version 12.0

6fc6872

[UPD] Update partner_multi_relation.pot

5bf14c3

Update translation files

5e587b6

Updated by Update PO files to match POT (msgmerge) hook in Weblate.

[UPD] Update partner_multi_relation.pot

aaefc62

[UPD] README.rst

af2757c

[FIX] Context Errors With Active ID

1891c7b

[IMP] Flake8

[IMP] Formatting

27022c3

[FIX] remove import statements for tests

d16752b

rousseldenis added this to the 13.0 milestone Jul 7, 2020

HaraldPanten requested changes Jul 9, 2020

View reviewed changes

pedrobaeza changed the title ~~[13.0] [MIG] partner multi relation~~ [13.0] [MIG] partner_multi_relation Sep 7, 2020

rousseldenis approved these changes Sep 7, 2020

View reviewed changes

regispirard approved these changes Sep 7, 2020

View reviewed changes

OCA-git-bot added a commit that referenced this pull request Sep 7, 2020

Merge PR #936 into 13.0

b707ded

Signed-off-by pedrobaeza

OCA-git-bot added the bot is merging ⏳ label Sep 7, 2020

OCA-git-bot mentioned this pull request Sep 7, 2020

Migration to version 13.0 #783

Open

47 tasks

OCA-git-bot removed the bot is merging ⏳ label Sep 7, 2020

OCA-git-bot added the bot is merging ⏳ label Sep 8, 2020

OCA-git-bot merged commit c670d46 into OCA:13.0 Sep 8, 2020

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[13.0] [MIG] partner_multi_relation #936

[13.0] [MIG] partner_multi_relation #936

NL66278 commented Jun 29, 2020

pedrobaeza commented Jun 30, 2020

NL66278 commented Jun 30, 2020

pedrobaeza commented Jun 30, 2020

NL66278 commented Jun 30, 2020

pedrobaeza commented Jun 30, 2020

pedrobaeza commented Jun 30, 2020

HaraldPanten left a comment •

edited

Loading

NL66278 commented Jul 9, 2020

BT-jstein commented Aug 20, 2020

pedrobaeza commented Sep 7, 2020

rousseldenis left a comment

regispirard left a comment •

edited

Loading

pedrobaeza commented Sep 7, 2020

OCA-git-bot commented Sep 7, 2020

OCA-git-bot commented Sep 7, 2020

rousseldenis commented Sep 7, 2020 •

edited

Loading

pedrobaeza commented Sep 7, 2020

rousseldenis commented Sep 7, 2020

pedrobaeza commented Sep 7, 2020

rousseldenis commented Sep 7, 2020

pedrobaeza commented Sep 7, 2020

rousseldenis commented Sep 7, 2020

pedrobaeza commented Sep 7, 2020

pedrobaeza commented Sep 8, 2020

OCA-git-bot commented Sep 8, 2020

OCA-git-bot commented Sep 8, 2020

[13.0] [MIG] partner_multi_relation #936

[13.0] [MIG] partner_multi_relation #936

Conversation

NL66278 commented Jun 29, 2020

pedrobaeza commented Jun 30, 2020

NL66278 commented Jun 30, 2020

pedrobaeza commented Jun 30, 2020

NL66278 commented Jun 30, 2020

pedrobaeza commented Jun 30, 2020

pedrobaeza commented Jun 30, 2020

HaraldPanten left a comment • edited Loading

Choose a reason for hiding this comment

NL66278 commented Jul 9, 2020

BT-jstein commented Aug 20, 2020

pedrobaeza commented Sep 7, 2020

rousseldenis left a comment

Choose a reason for hiding this comment

regispirard left a comment • edited Loading

Choose a reason for hiding this comment

pedrobaeza commented Sep 7, 2020

OCA-git-bot commented Sep 7, 2020

OCA-git-bot commented Sep 7, 2020

rousseldenis commented Sep 7, 2020 • edited Loading

pedrobaeza commented Sep 7, 2020

rousseldenis commented Sep 7, 2020

pedrobaeza commented Sep 7, 2020

rousseldenis commented Sep 7, 2020

pedrobaeza commented Sep 7, 2020

rousseldenis commented Sep 7, 2020

pedrobaeza commented Sep 7, 2020

pedrobaeza commented Sep 8, 2020

OCA-git-bot commented Sep 8, 2020

OCA-git-bot commented Sep 8, 2020

HaraldPanten left a comment •

edited

Loading

regispirard left a comment •

edited

Loading

rousseldenis commented Sep 7, 2020 •

edited

Loading