Skip to content

fixup! Merge branch '8.0' into 8.0-base_domain_operator - #5

Merged
hbrunn merged 1 commit into
hbrunn:8.0-base_domain_operatorfrom
daramousk:8.0-fix-base_domain_operator
Mar 18, 2019
Merged

hbrunn merged 1 commit into
hbrunn:8.0-base_domain_operatorfrom
daramousk:8.0-fix-base_domain_operator

Conversation

@daramousk

Copy link
Copy Markdown

Definitely I think this can be written with less code but I did the minimal changes for now.

@hbrunn

hbrunn commented Mar 6, 2019

Copy link
Copy Markdown
Owner

we need to support both cases. This fails miserably if you construct a domain in code that uses ids. So inspect what we got, if it's a string do a name_search, and then most of the code can be unchanged

@hbrunn

hbrunn commented Mar 6, 2019

Copy link
Copy Markdown
Owner

...and add a test case for this too please

@hbrunn

hbrunn commented Mar 6, 2019

Copy link
Copy Markdown
Owner

...and write a proper commit message, as I won't squash away your work pretending it's mine

@daramousk
daramousk force-pushed the 8.0-fix-base_domain_operator branch from 37ec070 to 1b7b9ac Compare March 7, 2019 13:37
@daramousk

Copy link
Copy Markdown
Author

@NL66278 This is the pr you are looking for.

# https://github.com/OCA/OCB/blob/10.0/odoo/osv/expression.py#L738
def parent_of_domain(left, ids, left_model, parent=None, prefix=''):
def parent_of_domain(left, right, left_model, parent=None, prefix=''):
if isinstance(right, unicode):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why don't you just assign ids the ids resulting from name_get and leave the rest unchanged?

And testing for a string in python < 3 should be done by checking for basestring

])
self.assertTrue(substring_matches)
# see how _operator_parent_of deals with strings
leaf = self.env['base.domain.operator']._operator_parent_of(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

any reason to do complicated stuff here instead of just constructing a domain as above?

def parent_of_domain(left, ids, left_model, parent=None, prefix=''):
def parent_of_domain(left, right, left_model, parent=None, prefix=''):
if isinstance(right, basestring):
fun = left_model.name_search

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I still think you can do here ids = [_id for _id, dummy in left_model.name_search(ids)] and leave the rest of the file unchanged

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@hbrunn I am not sure that would be a good idea. While this will work for cases in which the ids is a string (from the web client, but it will fail when the ids is actually an array of integers.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

that's why you do this only under the scope of this conditional

@daramousk
daramousk force-pushed the 8.0-fix-base_domain_operator branch from 55bd80b to 616ee67 Compare March 14, 2019 13:33
def parent_of_domain(left, ids, left_model, parent=None, prefix=''):
if isinstance(right, basestring):
ids = [_id for _id, dummy in left_model.name_search(ids)]
else:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

what is this branch good for?

@daramousk
daramousk force-pushed the 8.0-fix-base_domain_operator branch from 616ee67 to 7b95ea5 Compare March 14, 2019 15:54

@hbrunn hbrunn left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

please remove the part in the readme where the issue you fix is mentioned, then we can merge

Comment thread base_domain_operator/tests/test_base_domain_operator.py Outdated
@daramousk
daramousk force-pushed the 8.0-fix-base_domain_operator branch from 7b95ea5 to cbe78e9 Compare March 15, 2019 15:46
@hbrunn
hbrunn merged commit 5b1a6b0 into hbrunn:8.0-base_domain_operator Mar 18, 2019
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.

2 participants