[16.0][IMP] Add back the variable feature in sql_export by florian-dacosta · Pull Request #718 · OCA/reporting-engine

florian-dacosta · 2023-02-03T10:39:41Z

This feature was removed during migration in order to migrate the module fast.
I add it back now, with a refactore, the module now uses the nice new "properties" field and we don't need to create manual fields anymore.
The use of the sql queries using variable should be easier

@bguillot @legalsylvain

OCA-git-bot · 2023-02-03T10:39:52Z

Hi @legalsylvain,
some modules you are maintaining are being modified, check this out!

legalsylvain

Really nice job !

I didn't know PropertiesDefinition. Looks great and more simple. (+114 / - 223 )

Partial code review & functional tests.

Question :
What is the objective of setting values in this view ?

I was expected it will be the default values, in the wizard, but it's not the case.
(But I just discover PropertiesDefinition, maybe my question is not relevant).

2 things :

could you make required field in the wizard ? if not, user can click on export, but then it raise an SQL error if one of the variable is empty.
psycopg2.errors.SyntaxError: syntax error at or near "false" LINE 7: AND rel.category_id in false) TO STDOUT WITH CSV HEADER DELI...
could you put a variable per line ? (no blocking point, but I see it more usable).

legalsylvain · 2023-02-03T11:02:00Z

+            if rec.query_properties_definition:
+                rec.use_properties = True
+            else:
+                rec.use_properties = False


Suggested change

if rec.query_properties_definition:

rec.use_properties = True

else:

rec.use_properties = False

rec.use_properties = bool(rec.query_properties_definition)

florian-dacosta · 2023-02-03T11:38:38Z

I didn't know PropertiesDefinition. Looks great and more simple. (+114 / - 223 )

It is more simple (at least once you get how properties field work, but as it is a standard odoo field now, seems ok)

Question : What is the objective of setting values in this view ?

This is how properties field work. On one object, you can create the property fields and set values (we want the values on the wizard to use it in the query). And when adding the properties, Odoo will store it in the related PropertiesDefinition field.
So on sql.export, we have the field that store the properties definition (name, type, default value...) and on the wizard sql.export.file, we have the properties field, which allow us to set different value on each export.

So the configuration of the properties have to be on the sql.file.wizard model.

I was expected it will be the default values, in the wizard, but it's not the case. (But I just discover PropertiesDefinition, maybe my question is not relevant).

Not sure I understood what you meant

* could you make required field in the wizard ? if not, user can click on export, but then it raise an SQL error if one of the variable is empty.

Well, this is the major draw back with the use of this properties field. I don't think it is possible to put it as required.
I believe it is kind of acceptable because if you set a variable on your query, you always want to fill a value. I don't really see use case where you would have an optional parameter for your query...
Maybe we could raise a clean UserError on validation if one of the property is empty ? I think it would be ok this way.
Anyway, in v16, required field are not even really visibible with default theme !

* could you put a variable per line ? (no blocking point, but I see it more usable).

Done

florian-dacosta · 2023-02-03T11:47:38Z

For the required field question, IMO, we can put it in the known issues/ROADMAP that we don't manage optional fields, only required (even if not visible in the view) we raise is a property is empty, and if Odoo improve the properties field with the required property, then we'll improve it.

legalsylvain · 2023-02-03T12:12:42Z

-                    variable_dict[field.name] = self[field.name].id
-                elif field.ttype == "many2many":
-                    variable_dict[field.name] = tuple(self[field.name].ids)
+        if self.query_properties:


useless line, as the following line is a for.

legalsylvain · 2023-02-03T12:19:08Z

I was expected it will be the default values, in the wizard, but it's not the case. (But I just discover PropertiesDefinition, maybe my question is not relevant).

Not sure I understood what you meant

Well : step to reproduce.

go to a sql.export item.
click on "Configure Properties"
set default values

Save
Go back
click again on "Configure Properties"
-> values are lost.

Run the export.
-> the values are not recovered.

florian-dacosta · 2023-02-03T13:31:52Z

Well : step to reproduce.

* go to a sql.export item.

* click on "Configure Properties"

* set default values

You did not set default values for the properties here, you just set a value, for the current wizard.
To set a default value on the property, you have to edit the property (the definition of the property) with the pen like in the following image :

Once you have set the default value on the property, it will be filled by defaut with the execute query wizard or even if you go back on the configuration (which is exactly the same thing actually).

github-actions · 2023-06-04T12:24:51Z

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

Refactore to use the new properties field

florian-dacosta · 2023-06-05T08:03:00Z

Hi @legalsylvain Would you mind updating your review ?

@bguillot could you do a review on this one ?

legalsylvain · 2023-06-20T08:38:57Z

Hi @florian-dacosta. Just tested the module. all is great, except the error when user doesn't enter all the values :

I made a PR against akretion branch. could you take a look ?

legalsylvain

akretion#3

…nagement-IMP 16 sql export fix variable management imp

florian-dacosta · 2023-06-20T09:32:11Z

akretion#3

Thanks for the improvement, it is merged!

legalsylvain

Thanks !
Could you ask another review from akretion team ?

bealdav

LGTM

... on friday with accounting made, yeoh

enjoy

legalsylvain

It requires migration scripts. Dont you think ?

florian-dacosta · 2023-10-10T09:58:08Z

It requires migration scripts. Dont you think ?

More or less..
Sure, it is always nice to have a migration script, but here :
The module is already merged for some month without this feature (so the problem of this variable feature not working has been going on for months...)
The script seems quite complicated to me
Without the migration script, installation won't break, the user will only have to create the variable himself => He needs to get familiar with the new system anyway.
The user will have Manual action on migration anyway, because we can't migrate the content of the query. Since the user has to re-write the query (or at least check it), it seems legit to me that he also has a look at the variable.
Finally, the way the variable were named before was like "x_my_field" as it was using manual fields in Odoo, and with the new system, you name it as you want to, so it is the opportunity to make a clean naming.

But mainly for the fact that one have to re-write/re-check the query anyway and the complication to do the script make me think we should not bother on this one.
What do you think @legalsylvain ?

legalsylvain · 2023-10-10T14:25:36Z

Let's go ahead.

/ocabot merge major

OCA-git-bot · 2023-10-10T14:26:25Z

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-718-by-legalsylvain-bump-major, awaiting test results.

OCA-git-bot · 2023-10-10T14:35:34Z

Congratulations, your PR was merged at 24305cd. Thanks a lot for contributing to OCA. ❤️

legalsylvain requested changes Feb 3, 2023

View reviewed changes

florian-dacosta force-pushed the 16-sql-export-fix-variable-management branch from a681d50 to e675732 Compare February 3, 2023 11:27

legalsylvain reviewed Feb 3, 2023

View reviewed changes

github-actions Bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 4, 2023

[IMP] Add back the variable feature in sql_export

741b22c

Refactore to use the new properties field

florian-dacosta force-pushed the 16-sql-export-fix-variable-management branch from e675732 to 741b22c Compare June 5, 2023 08:00

github-actions Bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 11, 2023

florian-dacosta requested a review from legalsylvain June 19, 2023 08:35

legalsylvain mentioned this pull request Jun 20, 2023

16 sql export fix variable management imp akretion/reporting-engine#3

Merged

legalsylvain requested changes Jun 20, 2023

View reviewed changes

legalsylvain and others added 3 commits June 20, 2023 11:27

[IMP] sql_export : force user to set properties to avoid server error

8adf34f

[REF] sql_export : remove obsolete argument in actions dicts

e37107a

Merge pull request #3 from legalsylvain/16-sql-export-fix-variable-ma…

3476c06

…nagement-IMP 16 sql export fix variable management imp

legalsylvain approved these changes Jun 20, 2023

View reviewed changes

bealdav approved these changes Sep 29, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 29, 2023

legalsylvain reviewed Sep 29, 2023

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Oct 10, 2023

OCA-git-bot merged commit ace7828 into OCA:16.0 Oct 10, 2023

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Oct 10, 2023

florian-dacosta mentioned this pull request Nov 28, 2023

[16][FIX] sql_export : remove unused action parameter #820

Merged

Uh oh!

Uh oh!

Conversation

florian-dacosta commented Feb 3, 2023

Uh oh!

OCA-git-bot commented Feb 3, 2023

Uh oh!

legalsylvain left a comment

Choose a reason for hiding this comment

Uh oh!

legalsylvain Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

florian-dacosta commented Feb 3, 2023

Uh oh!

florian-dacosta commented Feb 3, 2023

Uh oh!

legalsylvain Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

legalsylvain commented Feb 3, 2023

Uh oh!

florian-dacosta commented Feb 3, 2023

Uh oh!

github-actions Bot commented Jun 4, 2023

Uh oh!

florian-dacosta commented Jun 5, 2023

Uh oh!

legalsylvain commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legalsylvain left a comment

Choose a reason for hiding this comment

Uh oh!

florian-dacosta commented Jun 20, 2023

Uh oh!

legalsylvain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bealdav left a comment

Choose a reason for hiding this comment

Uh oh!

legalsylvain left a comment

Choose a reason for hiding this comment

Uh oh!

florian-dacosta commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legalsylvain commented Oct 10, 2023

Uh oh!

OCA-git-bot commented Oct 10, 2023

Uh oh!

OCA-git-bot commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

legalsylvain commented Jun 20, 2023 •

edited

Loading

legalsylvain left a comment •

edited

Loading

florian-dacosta commented Oct 10, 2023 •

edited

Loading