[IMP] Add tests for base_location name_search

[WohthaN]

Continuing here the discussion/development of MR
#582
which was closed by mistake.

[pedrobaeza]

But the name_search is not modified in this PR...

[WohthaN]

Now it could be ok!

[pedrobaeza]

And change the module version number...

[pedrobaeza]

The problem with this PR is that you are breaking inheritance. The problem with name_search is that is very tricky. Check how we were force to do it in res.partner for l10n_es_partner:

https://github.com/OCA/l10n-spain/blob/59f39578f6de75a535c15e58428501257be2cf41/l10n_es_partner/models/res_partner.py#L34-L54

[WohthaN]

Now I'm really confused, what do you mean it breaks inheritance?

It hase the same interface as in models.Model, and it's actually implemented exactly in the same way as in models.Model, pretty much line by line (it just skips access rights tests that in this case are not necessary)!

What am I missing?

For reference, current models.Model implementation is:

    @api.model
    def _name_search(self, name='', args=None, operator='ilike', limit=100, name_get_uid=None):
        # private implementation of name_search, allows passing a dedicated user
        # for the name_get part to solve some access rights issues
        args = list(args or [])
        # optimize out the default criterion of ``ilike ''`` that matches everything
        if not self._rec_name:
            _logger.warning("Cannot execute name_search, no _rec_name defined on %s", self._name)
        elif not (name == '' and operator == 'ilike'):
            args += [(self._rec_name, operator, name)]
        access_rights_uid = name_get_uid or self._uid
        ids = self._search(args, limit=limit, access_rights_uid=access_rights_uid)
        recs = self.browse(ids)
        return recs.sudo(access_rights_uid).name_get()

[pedrobaeza]

That's the base model. When you don't call super in your module, and there's other module overwriting the same method, both conflict and you don't know which of the 2 is going to be executed.

[WohthaN]

Your argument is actually true for each and every method in a class, and for the little I understand it's a problem intrinsic to the the multi-inheritance scheme odoo is using which bypasses normal pythonic inheritance (in a sane world, inheritance should be predictable, deterministic and hopefully in cortrol of the programmer).

The solution you pointed in l10n-spain is not really solving the problem you described: it's a workaround that actually leaves you with partially undefined behaviour anyway, since you don't know what results you get first if someone else is exending ResPartner again, in the end virtually any subclass is adding search results in a potentially random order and nobody knows what's really coming out of that name_search call. Besides, in that example super is called only if the new search is not producing enough results, not even everytime, so the original behaviour is not always maintained.

Moreover, in the example you pointed out, an existing model, ResPartner, is being extended, hence you do want to extend rather than replace the behaviour of the model being extended. BetterZip is not extending an existing model, it's introducing a new one, so this class is actually the one defining the behaviour of this model, not the one extending it. Your argument could be applied to any class exending BetterZip though...

[pedrobaeza]

You are totally right, as the model is first defined here! Sorry for the noise.

[WohthaN]

No worries, and thanks for the thorough review! Your comments forced me to go back to check and learn lots of stuff i overlooked, which in the end is really really helpful! :)