-
-
Notifications
You must be signed in to change notification settings - Fork 796
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
[13.0][ADD] report_dynamic - Lets Odoo users build reports #591
base: 13.0
Are you sure you want to change the base?
Conversation
@Kiplangatdan Opened it slightly differently |
@thomaspaulb This is alright, it can be picked from here. |
Hey @thomaspaulb, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
CLA for @douglas-tabut is OK (checked manually, I'll look into why the clabot is not finding it) |
@gurneyalex Thanks for checking! |
Depends on OCA/web#1822 which is not merged yet. |
Now merged. |
@thomaspaulb let's please set this to draft until we have something complete |
|
@thomaspaulb great PR! |
Hi @thomaspaulb , do you plan on working further on this one? |
@francesco-ooops Yes! There only two todo's left:
Hey @ntsirintanis, this is on your plate but it looks like you didn't get to it yet - do you need more pointers on this one? If yes, reassign back to me, if no, let's plan some time to get this over the finish line. |
@thomaspaulb great PR! any good news about these two points ? |
@thomaspaulb could you also do a rebase? :) |
329cbed
to
df81c7c
Compare
I am going to continue on this when I manage to find some time. |
TODO: One window action per model |
523d968
to
b88619c
Compare
TODO: provide search view, add column resource_ref in tree view, hide it when there is a template. |
@api.model | ||
def _selection_target_model(self): | ||
models = self.env["ir.model"].search([]) | ||
return [(model.model, model.name) for model in models] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check here if there's an efficient way to present only those models that at least one active record exists in the db.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just simply get the distinct model ids or names from the report dynamic table first, and then use them as filter criteria in this search domain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course you could also refactor this selection field to a many2one to ir.model which might make it a bit easier all in all but not sure if its worth it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's what I am attempting to do, to get non transient models with count > 0. Will resume working on this soon, most probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea in one step thats difficult, you could do that with sql but thats definitely not worth it:) if its two steps you can just unique them in python and the ones in report dynamic will be non transient anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah well maybe if you refactor to a many2one and then use read_group :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably replacing the reference field with a many2one to ir.model, and another many2one to ir.model.model, will make this easier. Something to consider in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would start by filtering out non real models. Then you can add a TODO to improve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SELECT
nspname AS schemaname,relname,reltuples
FROM pg_class C
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE
nspname NOT IN ('pg_catalog', 'information_schema') AND
relkind='r'
ORDER BY reltuples DESC;
the above takes the amount of tuples from the statistics (created by autovacuum) - but I consider only showing models with records pretty much an anti-feature. I'd want to be able to prepare a report even if I haven't created records already, and also installing some module using this to create a report would fail if the model doesn't have records
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this?
4eed04d
to
8bf8845
Compare
I've run out of ideas of how to make this (functionally) better. If anyone's feels like doing some functional testing and bring forth also some ideas, please do so. I will start thinking about how to unit test everything here. |
8bf8845
to
78b6052
Compare
Hi @thomaspaulb and @ntsirintanis really thanks for this PR!!! This is just my review from the UX point of view: 1 Create new Tab “Dynamic Placeholder” for all fields about this argument 2 Move the field “Preview Record” above 3 Enable/Disable expression Python (with the switch widget) when it’s disabled then hide the description 4 Archive in the first place as the standard 5 Why i need this list in the form when i can manage and create my sections directly from smart button ? Please let me know your opinion of these points. |
@elvise thanks a lot for taking the time to test this, and respond with some really meaningful feedback! I'll go through your points and implement them as needed. |
@ntsirintanis I tested again, i think we need to add a new menu “Report” because right now its reachable only if you click in the name “Dynamic Reports” RPReplay_Final1655643072.mov@ntsirintanis what do you think ? |
Hi @ntsirintanis do you already have some ideas on how to manage o2m fields such as "sale orde line"? |
…view_resource_ref
@ntsirintanis I did a lot of reshuffling of functionality, the module as a whole seems more logical to me. It still needs some love and testing though - I think I have broken the instant preview functionality that existed on individual sections, and although the PR description claims to include |
0b2662e
to
c51fe59
Compare
|
||
template_id = fields.Many2one( | ||
comodel_name="report.dynamic", | ||
domain=lambda self: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaspaulb The idea is to make this domain computed. The "standard" part of the domain will be the requirements that the selected record is a template, with model_id == active_model
. Then, we loop on all templates in the database for that model_id. In the computation of the domain we add the "standard" part, and OR
every template.condition_domain_global
. Then user can select only a template that it's computed domain selects all active_ids. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, however implementationwise, the easiest is to make an invisible, computed field matching_template_ids
on the wizard, and then on the XML of the form view, not on the Python field, add a domain of "id, in, matching_template_ids"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works only when matching_template_ids is an (invisible) field on the form view too.
The only drawback of this method is it can get really slow when the number of matching records is > 10000 or something, in which case you would resort to OCA's web_domain_field
instead. But for matching templates, I don't expect 10000 :-) just a handful max
@api.model | ||
def _selection_target_model(self): | ||
models = self.env["ir.model"].search([]) | ||
return [(model.model, model.name) for model in models] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SELECT
nspname AS schemaname,relname,reltuples
FROM pg_class C
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE
nspname NOT IN ('pg_catalog', 'information_schema') AND
relkind='r'
ORDER BY reltuples DESC;
the above takes the amount of tuples from the statistics (created by autovacuum) - but I consider only showing models with records pretty much an anti-feature. I'd want to be able to prepare a report even if I haven't created records already, and also installing some module using this to create a report would fail if the model doesn't have records
[("model", "=", "ir.ui.view"), ("res_id", "=", self.wrapper_report_id.id)], | ||
limit=1, | ||
) | ||
return "{}.{}".format(record.module, record.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use https://github.com/OCA/OCB/blob/13.0/odoo/addons/base/models/ir_ui_view.py#L235 here
but what happens if I want to use a template without xmlid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not answered question?
lock_date = fields.Date(readonly=True) | ||
field_ids = fields.Many2many( | ||
comodel_name="ir.model.fields", | ||
relation="contextual_field_rel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to start this with report_dynamic_...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this?
def action_lock_report(self): | ||
"""Lock the report on given date""" | ||
self.ensure_one() | ||
self.report_id.lock_date = self.lock_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to generate the report(s) at this point and attach them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a TODO comment?
that's not true, right? But adding this server action would be a nice usability thing |
@@ -0,0 +1,70 @@ | |||
<?xml version="1.0" encoding="utf-8" ?> | |||
<odoo noupdate="1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure about using noupdate for demo data?
if not model: | ||
return res | ||
try: | ||
self.env[model].search_read([], limit=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we avoid reading here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use
self.env[model].search_read([], limit=1, fields=['display_name'])
BUT
with reading all the fields we can be sure, we have access to any field, we can use in reporting. For current person at least.
So it can be fields arg added, or stay as it is with such a "check".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get what you are trying to achieve here and why you want to load ALL fields for ALL records.
If the current user is not able to read a field does not mean that users using the report won't be able to...
@@ -0,0 +1,26 @@ | |||
class Header: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class Header: | |
class Header: | |
__slots__ = ("value", "base_value", "child") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this?
It's sort of true - the popup was supposed to come only when multiple templates match, but currently it always comes and people have to choose the template. |
@thomaspaulb just a gentle reminder :) |
<?xml version="1.0" encoding="utf-8" ?> | ||
<odoo noupdate="1"> | ||
<!-- template for demo_report_1 --> | ||
<record id="demo_template_1" model="report.dynamic"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know it is my problem only, but noupdate flag is ignored and second test fire up is blocked by _constrain_template_status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normally demo data is loaded only w/ -i
. Anyway, my comment above is not blocking.
[IMP]Test Case type changed to SavepointCase
Hi all, happy to see it works now. Thanks for that. |
@simahawk gentle reminder :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of not answered comments.
Feel free to reply with "I add a TODO for later" if you don't have time and want to move fwd.
Regarding commits: do you need them all? I think you can squash all in one when done.
Other than this, LGTM.
@@ -105,7 +105,7 @@ repos: | |||
rev: 2.5.2 | |||
hooks: | |||
- id: setuptools-odoo-make-default | |||
- repo: https://gitlab.com/pycqa/flake8 | |||
- repo: https://github.com/pycqa/flake8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls rebase after #726 is merged and trash this change
<?xml version="1.0" encoding="utf-8" ?> | ||
<odoo noupdate="1"> | ||
<!-- template for demo_report_1 --> | ||
<record id="demo_template_1" model="report.dynamic"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normally demo data is loaded only w/ -i
. Anyway, my comment above is not blocking.
lock_date = fields.Date(readonly=True) | ||
field_ids = fields.Many2many( | ||
comodel_name="ir.model.fields", | ||
relation="contextual_field_rel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this?
@api.model | ||
def _selection_target_model(self): | ||
models = self.env["ir.model"].search([]) | ||
return [(model.model, model.name) for model in models] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this?
if not model: | ||
return res | ||
try: | ||
self.env[model].search_read([], limit=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get what you are trying to achieve here and why you want to load ALL fields for ALL records.
If the current user is not able to read a field does not mean that users using the report won't be able to...
[("model", "=", "ir.ui.view"), ("res_id", "=", self.wrapper_report_id.id)], | ||
limit=1, | ||
) | ||
return "{}.{}".format(record.module, record.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not answered question?
@@ -0,0 +1,26 @@ | |||
class Header: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this?
def action_lock_report(self): | ||
"""Lock the report on given date""" | ||
self.ensure_one() | ||
self.report_id.lock_date = self.lock_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a TODO comment?
@thomaspaulb what is your plan for this great PR? |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
This new module allows Odoo users to create a report on any model without the need of a developer: