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] partner multi relation extendable #497

Merged
merged 4 commits into from
Apr 13, 2018

Conversation

NL66278
Copy link
Contributor

@NL66278 NL66278 commented Oct 16, 2017

Make it easy and reliable to show all kinds of relations between partners. Not only those entered manually, but also between partners and their contact persons/addresses. Of betwee customers and their sales persons.
The changes to this module provide the flexibility on which extension modules can build.

@hbrunn
Copy link
Member

hbrunn commented Oct 16, 2017

consider using git commit --fixup=$hash_of_previous_commit. Because then, you don't need to fill in a commit message and, rebasing is simply git rebase -i $hash_of_previous_commit~1 --autosquash

@NL66278 NL66278 force-pushed the 10.0-partner_multi_relation-extendable branch 5 times, most recently from 26dbac2 to 392e200 Compare October 19, 2017 17:02
@NL66278 NL66278 force-pushed the 10.0-partner_multi_relation-extendable branch from b6458eb to 72297e6 Compare October 28, 2017 16:09
@NL66278 NL66278 changed the title [10.0][WIP] partner multi relation extendable [10.0] partner multi relation extendable Oct 29, 2017
@NL66278 NL66278 force-pushed the 10.0-partner_multi_relation-extendable branch 4 times, most recently from eb6964d to 579131e Compare November 2, 2017 13:56
@rousseldenis
Copy link
Contributor

@NL66278 As partner_multi_relation is on 10.0 branch, could you rebase all your PR's concerning that module ?

We could see more clearly what changes are involved in them and move forward.

Thanks

@NL66278 NL66278 force-pushed the 10.0-partner_multi_relation-extendable branch from 579131e to 8145e14 Compare February 8, 2018 14:18
@ghost
Copy link

ghost commented Feb 12, 2018

@NL66278 Thank you, LGTM 👍

@NL66278 NL66278 mentioned this pull request Feb 22, 2018
53 tasks
@NL66278
Copy link
Contributor Author

NL66278 commented Apr 10, 2018

@hbrunn @rousseldenis @dufresnedavid Now that this PR is used for the migration to 11.0 it might be time to merge it in 10.0? Not only is this change the base for extention modules, but some annoying buds have been solved and tests have been extended.

@ddufresne
Copy link
Contributor

Yes, let's merge this pull request.

@NL66278
Copy link
Contributor Author

NL66278 commented Apr 10, 2018

@ddufresne For a merge we need two approvals. Please go to the changes tab and use the button 'Review Changes'

_logger = logging.getLogger(__name__)


_last_key_offset = -1
Copy link
Member

Choose a reason for hiding this comment

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

I still disagree with this whole construction, please read http://effbot.org/zone/thread-synchronization.htm if you won't take it from me.

Why can't you do something similar as in https://github.com/OCA/OCB/blob/10.0/addons/account/report/account_invoice_report.py#L94? this then also is very much in line with the way Odoo inheritance works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hbrunn What happens in account_invoice_report is not really applicable, because there they have a single monolitic SQL statement, where inheriting modules can add something to the where or the from or the whatever. In my case all relations are a UNION of SQL statements (ofcourse together they are one statement), that each can have their own joins, wheres, whatevers, if only in the end the select the right number of columns of the right type. And ofcourse they should be a link from one partner to another. Apart from the modules already realized you could link managers to employees, salesmen to customers, any relation between partners you think are interesting to show as a connection.

As for the dangers of multithreading in combination with global variables I need no convincing. But as far as I can see module loading is not a multithreaded proces. Otherwise really wierd things could happen when for instance the ordering of model inheritance would become completely inpredictable.

Actually module loading itself uses some global variables. Like the quite important loaded array in modules/module.py, containing the modules already loaded. How does this differ from what I do?

Copy link
Member

Choose a reason for hiding this comment

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

by virtue of https://github.com/OCA/OCB/blob/10.0/odoo/modules/registry.py#L68

But let's not argue this point too much, I shouldn't have started with this most interesting argument, but the most compelling one: Global variables holding state will blow up when you have multiple databases in a threaded server. Consider what happens when db1 has two modules doing this, while db2 has one. Or none, doesn't matter. Don't you remember the times when people used globals all over the place and things went south constantly?

My example was just that, an example. For your case, I guess I'd do something like

def _get_statements(self):
    return ['select * from table']

def _get_statement(self):
    cr.execute(' union '.join(self._get_statements))

or something similar. Overrides then append to the list. Given you have probably quite some code using this already, _get_statements could just return a list of the dicts you're using anyways, then you don't have to change much code but still are thread safe.

@NL66278 NL66278 force-pushed the 10.0-partner_multi_relation-extendable branch 2 times, most recently from 244bbe8 to 52a8925 Compare April 12, 2018 15:47
@NL66278 NL66278 force-pushed the 10.0-partner_multi_relation-extendable branch from 52a8925 to a628d6a Compare April 12, 2018 21:37
@NL66278
Copy link
Contributor Author

NL66278 commented Apr 12, 2018

@hbrunn I hope you can be satisfied with the changes in the PR.

@hbrunn
Copy link
Member

hbrunn commented Apr 13, 2018

some elaborate comment in the function docstring how people are supposed to override would be nice, but that can come later too. thanks. I count this message as approval so we can merge.

@hbrunn hbrunn merged commit df64739 into OCA:10.0 Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants