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

[16.0][IMP+FIX] spreadsheet*_oca: New field to name the pivot table and fixed error adding to new sheet of dashboard #38

Merged
merged 6 commits into from
Jul 25, 2024

Conversation

CarlosRoca13
Copy link
Contributor

cc @Tecnaiva TT50012 and TT50206

ping @pedrobaeza @chienandalu

The error is happening because the dashboard where
the data has to be added is not being showed.

With this changes the field to define the
dashboard is required and is showed.

Added a way to limit the search to the dashboards
that can be edited
@CarlosRoca13
Copy link
Contributor Author

Added improvements to allow generate dynamic tables as showed in next gif and added an option to download the spreadsheet as xlsx (this option is available inside a spreadsheet on menu item File)
dynamic_table_example

@carolinafernandez-tecnativa

Thanks for the improvements :) Some comments:

1- I have checked and when downloading XLS is not respeting spreadsheet format.

Example:

image image

2- Dynamic field i think it must be visible only when trying to generate spreadhseet from pivot view, as if we try to create in tree view form is not working.

image

3- Database source is not filling automatically when creating spreadhseet from tree view. What should i add in this field?

image

image

@CarlosRoca13
Copy link
Contributor Author

1- I have checked and when downloading XLS is not respeting spreadsheet format.

The XLSX data is generated by the core module. If you check the behavior in the odoo enterprise module, you will see that the result is the same.

For other comments I will do the changes, thanks :)

@CarlosRoca13
Copy link
Contributor Author

@carolinafernandez-tecnativa changes done 😄

The value of Datasource Name is the name that will be filled to identify the table on the spreadsheet.

Comment on lines +53 to +63
no_edit_ids = (
self.env["ir.model.data"]
.search(
[
("model", "=", self._name),
("module", "!=", "__export__"),
("noupdate", "=", 0),
]
)
.mapped("res_id")
)
Copy link
Member

Choose a reason for hiding this comment

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

I created a brand new dashboard but I can't make it to show up in the wizard. Am I doing something wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to add a spreadsheet to the panel to be able to show it

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I can't select any dashboard in the wizard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example

I am able to select it.

Copy link
Member

Choose a reason for hiding this comment

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

I was trying to dashboard new sheet. I was expecting to let me choose a dashboard to wich apend a sheet. Did I understand it wrong?

Copy link
Contributor Author

@CarlosRoca13 CarlosRoca13 Jul 19, 2024

Choose a reason for hiding this comment

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

Yes 😅 that option is to add a sheet to a created spreadsheet for the dashboard

Example:
new sheet example

This option can be used to set data that has to be hidden but it has to be used to make some operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that option that you say can be set, I will see how to add it

Thanks 😄

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok. See? I was getting it wrong 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new option to create new dashboards 😄

On dashboards exists tables that are updated dynamically,
with this changes we have the option to generate this
dynamic tables using the pivot inserter.

Added a button to duplicate and change to dynamic
table the selected table on spreadsheet editor.

If a table with multiple row goups generated, the
way to generate the dynamic table will be set the
number of rows for each level of indentation.

Example:
2 rowGroupBys
3 number of rows
- val1
    - subval1.1
    - subval1.2
    - subval1.3
- val2
    - subval2.1
    - subval2.2
    - subval2.3
- val3
    - subval3.1
    - subval3.2
    - subval3.3
@CarlosRoca13 CarlosRoca13 force-pushed the 16.0-FIX-spreadsheet_oca-order branch 3 times, most recently from c189796 to 396e2b5 Compare July 19, 2024 12:43
Since this changes the field datasource_name does not work with
tree and graph. Now the datasource_name is set as data name for the
created data.

Furthermore, the capability to create dynamic tables has no sense on
tree and graph, so it has been limited for pivots.
@CarlosRoca13 CarlosRoca13 force-pushed the 16.0-FIX-spreadsheet_oca-order branch from 396e2b5 to 5a3292d Compare July 19, 2024 12:45
@CarlosRoca13
Copy link
Contributor Author

The tables imported from the tree are dynamic by nature. So I have made some changes so that the dynamic selector is selected by default and that the value of the number of rows is filled with the value that was used in the threshold

Example:
tables

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.

Tested and working.

Thanks for all the fixes and the new excellent features!

Copy link

@AntoniRomera AntoniRomera left a comment

Choose a reason for hiding this comment

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

LGTM
We have in mind to start the migration to 17.0, if you merge this PR we could use it as a starting point.

Copy link

Choose a reason for hiding this comment

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

Functional review LGTM :) Thanks!

@pedrobaeza
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-38-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4c76272 into OCA:16.0 Jul 25, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 16.0-FIX-spreadsheet_oca-order branch July 25, 2024 16:52
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