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

Make some _reports_menu methods a little more readable #5501

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

skateman
Copy link
Member

Using the MiqReport.for_user method is not necessary for the default_reports_menu at all, as it only requests the Default reports regardless of the current user. I also expanded the code to more lines with some comments to make it easier to read.

Retrieving the custom reports in get_reports_menu can be also simplified by dropping the MiqReport.for_user and just asking for the reports for that one given group, instead of all groups of the user and then filtering. If the user have global access to all custom reports, the group_id is simply not passed to the where hash.

Ideally I'd like to extract these things into named scopes or at least class methods into the MiqGroup or MiqReport models, but first let's simplify them.

@miq-bot add_label refactoring, hammer/no, reporting
@miq-bot add_reviewer @mzazrivec
@miq-bot add_reviewer @h-kataria

@miq-bot
Copy link
Member

miq-bot commented Apr 29, 2019

@skateman Cannot apply the following label because they are not recognized: reporting

# If the current_user is not a report admin, restrict this to the current group only
query[:miq_group_id] = current_group.try(:id) unless current_user.report_admin_user?
# Add the custom reports in the required format in their own menu item
reports.push([@sb[:grp_title], [[_("Custom"), MiqReport.where(query).pluck(:name)]]])
Copy link
Member

@martinpovolny martinpovolny Apr 29, 2019

Choose a reason for hiding this comment

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

missing .order(:name)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@martinpovolny
Copy link
Member

The code looks good except for the missing .order(:name) (see above).

@skateman skateman force-pushed the reports-menu-refactor branch from 00752b0 to 1b06557 Compare April 29, 2019 11:31
@miq-bot
Copy link
Member

miq-bot commented Apr 29, 2019

Checked commit skateman@1b06557 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Contributor

@h-kataria h-kataria left a comment

Choose a reason for hiding this comment

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

LGTM, verified in UI.

@h-kataria h-kataria self-assigned this Apr 29, 2019
@h-kataria h-kataria added this to the Sprint 110 Ending Apr 29, 2019 milestone Apr 29, 2019
@h-kataria h-kataria merged commit 4c79865 into ManageIQ:master Apr 29, 2019
@skateman skateman deleted the reports-menu-refactor branch April 30, 2019 04:59
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.

None yet

4 participants