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

[MIG][REF] web_advanced_search: Rename, refactor, migrate #999

Closed
wants to merge 0 commits into from

Conversation

yajo
Copy link
Member

@yajo yajo commented Jul 20, 2018

  • Includes [11.0][MIG] web_advanced_search_x2x #984
  • Git history cleanup
  • Complete migration to v11
  • Refactor to use the new v11 decoupled widgets system
  • Advanced search is now a high-level feature from the filters menu; it simplifies code a lot, and the UX is even better
  • Split README system
  • Add fun to ROADMAP
  • Addon is renamed to web_advanced_search, since it enhaces the searching experience for all kind of fields now

@Tecnativa

@yajo yajo added this to the 11.0 milestone Jul 20, 2018
@yajo yajo self-assigned this Jul 20, 2018
@pedrobaeza pedrobaeza mentioned this pull request Jul 20, 2018
68 tasks
@yajo yajo force-pushed the 11.0-web_advanced_search_x2x-mig branch 2 times, most recently from bbf2f36 to 22b020c Compare July 20, 2018 09:50
@pedrobaeza pedrobaeza requested a review from JBF91 July 21, 2018 15:04
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Functionally, the only problem I find is with the resulting filter label, putting the technical domain. Isn't any way to put a text description of the filter like in previous versions happen?

@@ -0,0 +1,27 @@
# -*- coding: utf-8 -*-
# Copyright 2015 Therp BV <http://therp.nl>
# Copyright 2017 Tecnativa - Vicent Cubells
Copy link
Member

Choose a reason for hiding this comment

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

Add your copyright here

@@ -0,0 +1,283 @@
/* Copyright 2015 Therp BV <http://therp.nl>
Copy link
Member

Choose a reason for hiding this comment

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

Rename file according module renaming

@@ -0,0 +1,3 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Remove coding

@@ -0,0 +1,27 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Remove coding

Copy link

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

some details.
Simple tests OK

Search for x2x records in advanced search
=========================================

Standard behavior in advanced search for one2many, many2many and many2one fields is to do a `name_search`. This often is not satisfactionary as you might want to search for other properties. There might also be cases where you don't exactly know what you're searching for, then a list of possible options is necessary too. This module enables you to have a full search view to select the record in question, and either select specific records or select them using a search query of its own.

This comment was marked as resolved.

This comment was marked as resolved.


To use this module, you need to:

* open the advanced search options in a search view

This comment was marked as resolved.

This comment was marked as resolved.


To search for properties of linked records (ie invoices for customers with a credit limit higher than X):

* open the advanced search options in a search view

This comment was marked as resolved.

This comment was marked as resolved.

Known issues / Roadmap
======================

* When you use *is in selection* search system and choose a domain, it gets

This comment was marked as resolved.

This comment was marked as resolved.

"qweb": [
'static/src/xml/web_advanced_search.xml',
],
"auto_install": False,

Choose a reason for hiding this comment

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

no need for the keys with the default values no?

@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
<!-- Copyright 2017 Jairo Llopis <jairo.llopis@tecnativa.com>

Choose a reason for hiding this comment

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

update the date

Copy link
Member Author

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Functionally, the only problem I find is with the resulting filter label, putting the technical domain. Isn't any way to put a text description of the filter like in previous versions happen?

Yes, it's in the known issues. We'd need a tool to translate a domain to a human-readable format, which should possibly be upstream, either in odoo itself, or in the module that extends the domain widget, and then used downstream in this addon.

It doesn't seem as a blocker anyway, isn't it? 😇

Search for x2x records in advanced search
=========================================

Standard behavior in advanced search for one2many, many2many and many2one fields is to do a `name_search`. This often is not satisfactionary as you might want to search for other properties. There might also be cases where you don't exactly know what you're searching for, then a list of possible options is necessary too. This module enables you to have a full search view to select the record in question, and either select specific records or select them using a search query of its own.

This comment was marked as resolved.


To use this module, you need to:

* open the advanced search options in a search view

This comment was marked as resolved.


To search for properties of linked records (ie invoices for customers with a credit limit higher than X):

* open the advanced search options in a search view

This comment was marked as resolved.

Known issues / Roadmap
======================

* When you use *is in selection* search system and choose a domain, it gets

This comment was marked as resolved.

],
'installable': True,
"application": False,
}

This comment was marked as resolved.

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

code & functional review: LG.
Small things I'd change but not mandatory.

Tested on runbot: the "search more" is a bit glitchy as it closes the filter dropdown but it's something we can live with. Worth to drop a line in roadmap?


init: function () {
this._super.apply(this, arguments);
// To make widgets work, we need to a model and an empty record
Copy link
Contributor

Choose a reason for hiding this comment

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

*to add

/**
* A search field for relational fields.
*
* It implements and extends the `FieldManagerMixing`, and acts as if it
Copy link
Contributor

Choose a reason for hiding this comment

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

*Mixin

}
});
// Create dummy record with only the field the user is searching
var params = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this to a separated function

Copy link
Member Author

Choose a reason for hiding this comment

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

I have left this as it was. Thinking about it, it's only done once at init time, and calling it again could produce some problems, so it seems legit to not do a method about it IMHO.

I fixed your other comments though.

@yajo
Copy link
Member Author

yajo commented Aug 13, 2018

WIP until I include fixes for @simahawk's suggestions and I backport human-readable search string generator from odoo/odoo#25922.

Tested on runbot: the "search more" is a bit glitchy as it closes the filter dropdown but it's something we can live with. Worth to drop a line in roadmap?

That's something that I actually like, because when hitting "apply", you don't have to apply it again. What's the problem on that? 🤔

@yajo
Copy link
Member Author

yajo commented Aug 13, 2018

Nice, we have now human-readable leaf strings for advanced searches.

I was expecting odoo/odoo#25922 to get merged faster and me being able to backport it to OCB, but whatever. It's here in the addon now.

If other addons require this system, it could be split in a separate addon, but I think this is a good approach to keep it simple for now, when that's not happening.

This is how it looks now:
captura de pantalla de 2018-08-13 13-10-31

@pedrobaeza
Copy link
Member

@yajo excellent addition!

One nitpicking about the human readable format: please use character instead of > and remove spaces for better visualization and for differentiating from the operator "greater than". Are and and or words translatable?

@simahawk can you update your review?

@yajo
Copy link
Member Author

yajo commented Aug 16, 2018

Are and and or words translatable?

Yes.

@pedrobaeza pedrobaeza closed this Aug 16, 2018
@pedrobaeza pedrobaeza force-pushed the 11.0-web_advanced_search_x2x-mig branch from 76139cf to 3226b4c Compare August 16, 2018 15:50
@pedrobaeza
Copy link
Member

Merged in e764017

@pedrobaeza pedrobaeza deleted the 11.0-web_advanced_search_x2x-mig branch August 16, 2018 15:50
@JBF91
Copy link
Contributor

JBF91 commented Sep 13, 2018

Why manifest.py became openerp.py after merging?

@yajo
Copy link
Member Author

yajo commented Sep 13, 2018

😕 weird...

@pedrobaeza
Copy link
Member

Mea culpa, I screwed up on the squashing. Fixing directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants