Skip to content

[9.0][MIG][bi_view_editor] Ported BI View Editor module - #59

Merged
elicoidal merged 28 commits into
OCA:9.0from
onesteinbv:90_porting_bi_view_editor
Mar 13, 2017
Merged

elicoidal merged 28 commits into
OCA:9.0from
onesteinbv:90_porting_bi_view_editor

Conversation

@tarteo

@tarteo tarteo commented Jul 8, 2016

Copy link
Copy Markdown
Member

No description provided.

Comment thread bi_view_editor/README.rst Outdated

* Non-stored fields are not supported yet
* Provide graph view for table relations (would be a "nice to have")
* Porting to Odoo 9.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you can safely remove this sentence

@elicoidal

Copy link
Copy Markdown

@tarteo Thanks for the contribution!
Can you try to increase coverage with Unit test?

@lasley lasley added this to the 9.0 milestone Aug 15, 2016
@JordiBForgeFlow

Copy link
Copy Markdown
Member

@lreficent can you review and add test cases?

@LoisRForgeFlow

Copy link
Copy Markdown
Contributor

I've been trying to do some test but it seems to be a little complex, I am not being able to add proper data to test the action_create() method. Can you help us @tarteo?

@astirpe

astirpe commented Mar 2, 2017

Copy link
Copy Markdown
Member

@lreficent we experienced such issues too. It seems that creating a model while running tests raises some errors. Is that the case? Are you experiencing the same?

@JordiBForgeFlow

Copy link
Copy Markdown
Member

@astirpe yes. I think that it is not a good practice to introduce cr.commit here: https://github.com/OCA/reporting-engine/pull/59/files#diff-824d3c4f48b7f296e915d38877993ceaR274

Perhaps there are other ways to accomplish the same thing. Perhaps @hbrunn knows.

@astirpe

astirpe commented Mar 2, 2017

Copy link
Copy Markdown
Member

@jbeficent the issue is not the use of cr.commit, as you can notice we removed it while porting to V10 here. The problem is when creating a manual model during the tests.

Probably it is useful if you take a look at the travis failures while we were working on it https://github.com/onesteinbv/addons-onestein/commits/10.0/bi_view_editor. You can see a long list of attempts, about one month ago.

With version 9 we had the same issues as with V10.

@hbrunn

hbrunn commented Mar 2, 2017

Copy link
Copy Markdown
Member

@astirpe is this similar to the model purging tests in database_cleanup? After a lot of pain and misery, we decided to run these tests in its own run, because we couldn't figure out how to mess with the registry in tests and afterwards clean up in a way that doesn't affect the other modules: https://github.com/OCA/server-tools/blob/9.0/.travis.yml#L24-L27

@astirpe

astirpe commented Mar 2, 2017

Copy link
Copy Markdown
Member

@hbrunn I will check the database_cleanup module tomorrow. But I can already say that I'm quite sure that running the tests on its own env will solve this kind of issues. This is the case we've encountered: after running the bi_view_editor tests (successfully), the subsequent module being tested encounters gets troubles.

@hbrunn

hbrunn commented Mar 2, 2017

Copy link
Copy Markdown
Member

this was exactly the issue over there. And despite being theoretically and technically interesting to find out how to do this, I think after dedicating a couple of tens of hours to that without finding a good solution, I think it's okay to admit defeat for now

@JordiBForgeFlow

Copy link
Copy Markdown
Member

@hbrunn @astirpe OK, so can we merge this one now then?

@astirpe

astirpe commented Mar 3, 2017

Copy link
Copy Markdown
Member

@jbeficent I would like to update the code with a refactoring we already made. I will do it today, as soon as possible. After that I would be very happy if this PR is merged. Thanks to you all!

@astirpe

astirpe commented Mar 3, 2017

Copy link
Copy Markdown
Member

I've made a commit and of course I was expecting a failure of the coverage, since no tests are present. But here codecov says "No report found to compare against". Also runbot seems not responding, I just clicked on "Force Rebuild" button...

In the meanwhile does someone want to take a look at the latest changes in the code?

@astirpe

astirpe commented Mar 5, 2017

Copy link
Copy Markdown
Member

After understanding the job made for the tests in database_cleanup, finally I got travis successfully running with green unit tests! Special thanks to @hbrunn to mention that example!

Still I see codecov status "No report found to compare against". Still runbot not responding.
Does someone know the reason?

@LoisRForgeFlow LoisRForgeFlow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM now, the test were ran correctly in v10 btw. Thank you all!

@JordiBForgeFlow

Copy link
Copy Markdown
Member

@astirpe in this PR I introduced the tree view as a possible visualization option. At this moment the columns that appear in the list view are the ones of the BI model that you defined with the option "Row".

But in reality that is not 100% correct. The options "Row", "Column", "Measure" are intended to be used to define the output of the Pivot view.

Perhaps it would be better to have a flag 'List', that you can check on each field that you want to appear in the tree view. Perhaps even this flag should be set by default.

@astirpe

astirpe commented Mar 7, 2017

Copy link
Copy Markdown
Member

@jbeficent you're right! I could work on that next week or, if you want, you can open a PR that I will merge quickly. Thank you!

@JordiBForgeFlow

Copy link
Copy Markdown
Member

@astirpe I tried to do but did not succeed myself.

@astirpe

astirpe commented Mar 10, 2017

Copy link
Copy Markdown
Member

@jbeficent @lreficent With the latest commit we have the selection of the fields to be displayed in the tree view. Could you maybe take a look at it?

@astirpe

astirpe commented Mar 10, 2017

Copy link
Copy Markdown
Member

@lasley may I ask you to update the label of this PR, from "needs fixing" to "needs review"?

@elicoidal elicoidal left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have tested on runbot with 2 feedback

  • usability of cross model fields is misleading
  • a little glitch with the following example.

Test:

  • Added Currency + currency rate
  • At that moment a popup windows appears asking for options.

Issue 1: The usability is not obvious because user dont know whether to click on the link or on button OK (but we have no radio buttons in Odoo ...).
In my opinion the button should not be call OK but Cancel
Another option would be to use checkbox to emulate the radio buttons (along with a third option "None of the above").
Still those are simple suggestion about improving the usability and none blocking.

Issue 2: The glitch actually is that if you click on the OK button, the selected field is not added to the view (which is OK) but also disappears from the list on the left.
You have to save the view and re-enter the form to have the field back.

@astirpe

astirpe commented Mar 11, 2017

Copy link
Copy Markdown
Member

@elicoidal Thank you!

I've just committed the fix for Issue1, improving the user experience and making strings translatable. There is also a bit of js refactoring, based on the job already made while porting to V10. Would you like to see the new look&feel?

About Issue2, I tried to reproduce the glitch you described but it doesn't seem to occur to me. If I select a field and then I close the popup, that field remains at its place. Or maybe I misunderstood the issue?

@elicoidal

Copy link
Copy Markdown

@astirpe
Thanks for fixing issue1! Much clearer now :)
I have recorded the following to show the issue2:
bi_editor_disappearing_filed

@astirpe

astirpe commented Mar 12, 2017

Copy link
Copy Markdown
Member

@elicoidal thanks a lot!
Now I can understand the problem: you selected fields by drag&drop!
I was selecting fields by just clicking on the fields, that's why I was not able to reproduce the issue.

Working on it...

@elicoidal

Copy link
Copy Markdown

Now I can understand the problem: you selected fields by drag&drop!

My bad :p! Users are so unpredictable... ;)

@astirpe

astirpe commented Mar 12, 2017

Copy link
Copy Markdown
Member

@elicoidal I think this simple fix is enough, may I ask you check again?

@elicoidal

Copy link
Copy Markdown

👍

Comment thread bi_view_editor/README.rst Outdated

To graphically design your analysis data-set:

- From the Reporting menu, select "Custom BI Views"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the Dashboards menu

Comment thread bi_view_editor/README.rst Outdated
- From the Reporting menu, select "Custom BI Views"
- Browse trough the business objects in the Query tab
- Pick the interesting fields (Drag & Drop)
- For each selected field, right-click on the Options column and select whether it's a row, column or measure

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to remove the field from the list view, unflag the checkbox ´List´ in the Options column.

Comment thread bi_view_editor/models/bve_view.py Outdated
data = fields.Text(
help="Use the special query builder to define the query "
"to generate your report dataset. "
"NOTE: Te be edited, the query should be in 'Draft' status.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo: 'NOTE: To be edited'

Comment thread bi_view_editor/README.rst Outdated
- Save and click "Generate BI View"
- Click "Open BI View" to view the result
- If module Dashboard (board) is installed, the standard "Add to My Dashboard" functionality would be available
- Click "Create a menu" to create a new menu item directly linked to your new BI view (this feature is available in developer mode)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When the BI view is reset back to draft this menu will be removed, and you will need to re-create the menu entry.

@JordiBForgeFlow JordiBForgeFlow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Functional review

@elicoidal
elicoidal merged commit 3e71b4b into OCA:9.0 Mar 13, 2017
@astirpe

astirpe commented Mar 13, 2017

Copy link
Copy Markdown
Member

Yeeeeeee !!! Thank you all for your reviews, suggestions and interest !!!

@astirpe
astirpe deleted the 90_porting_bi_view_editor branch March 13, 2017 12:48
@JordiBForgeFlow

Copy link
Copy Markdown
Member

Thank you for this excellent contribution!!!

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.

8 participants