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

Reports: remove need to declare includes #13675

Merged
merged 2 commits into from
Feb 8, 2017

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jan 27, 2017

Blocked:

For reports, the columns are stated 2 times:

  • includes which defines the joined table columns.
  • cols which defines the database columns in the base table.
  • sort_cols which defines the joined table columns and the base table columns, and this is ordered.

This change focuses on the unnecessary includes. If the value is missing, the report simply derives the value from sort_cols.

The long term goal is to just specify the columns once (i.e. sort_cols) and not require includes or cols. (they would be supported, just not required) This will simplify extracting metadata from reports.

@lpichler Just double checking custom attributes with you. Pretty sure they are in cols and not includes but wanted to double check.

@kbrock kbrock changed the title Report both includes super Reports: remove need to declare includes Jan 27, 2017
@kbrock kbrock changed the title Reports: remove need to declare includes [WIP] Reports: remove need to declare includes Jan 27, 2017
@chessbyte chessbyte added the wip label Jan 27, 2017
@miq-bot
Copy link
Member

miq-bot commented Jan 27, 2017

This pull request is not mergeable. Please rebase and repush.

@kbrock kbrock changed the title [WIP] Reports: remove need to declare includes Reports: remove need to declare includes Jan 30, 2017
@kbrock kbrock removed the wip label Jan 30, 2017
@martinpovolny
Copy link
Member

Looks good to me.

Are there any impacts on the report editor? Guess there might be if we load a report w/o the includes.

Copy link
Member

@isimluk isimluk left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@lpichler lpichler left a comment

Choose a reason for hiding this comment

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

@lpichler Just double checking custom attributes with you. Pretty sure they are in cols and not includes but wanted to double check.

Yes, that's true. I am doing it in special method. (I'll check whether I can revamp it with your stuff)

@kbrock
Copy link
Member Author

kbrock commented Jan 31, 2017

@martinpovolny thanks. good eye.

I'm guessing include_for_find is not in the editor?
I will have to track down the report editor and check that out

@miq-bot
Copy link
Member

miq-bot commented Feb 7, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Feb 7, 2017

Checked commits kbrock/manageiq@400581d~...3af1aeb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

@kbrock
Copy link
Member Author

kbrock commented Feb 8, 2017

@martinpovolny Regarding the report editor
This PR is only for product/views, and nothing in product/reports, so this will not touch the reports editor.

@martinpovolny martinpovolny merged commit 9bdd3cd into ManageIQ:master Feb 8, 2017
@martinpovolny martinpovolny added this to the Sprint 54 Ending Feb 13, 2017 milestone Feb 8, 2017
@kbrock kbrock deleted the report_both_includes_super branch February 8, 2017 15:35
@kbrock
Copy link
Member Author

kbrock commented Mar 21, 2017

NOTE: reverting these report changes in #14439 - MiqReport#build_reportable_data is a little too complex for me to tackle right now. Safer to revert and address at a later time.

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.

7 participants