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

Identify invalid Catalog Items/Bundles in the UI #6448

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

hstastna
Copy link

@hstastna hstastna commented Nov 25, 2019

Issue: #6320
DEPENDS ON: ManageIQ/manageiq-decorators#26 (merged)

This PR:

  • adds a new column Valid in the list view to display whether the Catalog Item/Bundle is valid or not
  • displays warning as a flash message in the Catalog Item/Bundle details screen, if the item is invalid
  • makes Order button disabled for any invalid items

After:
Services > Catalogs > Catalog Items:
valid
Services > Catalogs > Service Catalogs:
valid_2
Services > Catalogs > Service Catalogs, details' screen of a selected Service:
invalid
Services > Catalogs > Catalog Items, details' screen of a selected Service:
invalid2

@miq-bot miq-bot added the wip label Nov 25, 2019
@hstastna
Copy link
Author

@miq-bot add_label services, enhancement

@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch 2 times, most recently from eabfb89 to e8b10bc Compare November 26, 2019 15:48
@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch 3 times, most recently from 6979c64 to cb5a189 Compare November 27, 2019 10:28
@hstastna hstastna changed the title [WIP] Identify invalid Catalog Items/Bundles in the UI Identify invalid Catalog Items/Bundles in the UI Nov 27, 2019
@miq-bot miq-bot added unmergeable and removed wip labels Nov 27, 2019
@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch from cb5a189 to 187f11e Compare December 3, 2019 12:20
@hstastna
Copy link
Author

hstastna commented Dec 3, 2019

@h-kataria This is ready for review. Thanks ❇️

@martinpovolny
Copy link
Member

Let's talk what you need to do and how to do it in a generic way.

Let's investigate there's / there's not already someone else doing it elsewhere.

If an exception is really needed from the generic behavior, let's put it closest to the place where it's actually needed.

@hstastna
Copy link
Author

@martinpovolny Thank you very much for the review. Yes, let's talk about this. I am not happy about my changes, too. And I haven't found any better possibilities yet.

There is some more info in #6320
Also, it looks like Laura was working on the same issue some longer time ago: ManageIQ/manageiq#4415

@hstastna
Copy link
Author

@miq-bot add_reviewer @skateman

@miq-bot miq-bot requested a review from skateman January 28, 2020 12:18
@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch from a3ec01d to e23761c Compare January 28, 2020 12:22
@hstastna hstastna changed the title Identify invalid Catalog Items/Bundles in the UI [WIP] Identify invalid Catalog Items/Bundles in the UI Jan 28, 2020
@miq-bot miq-bot added the wip label Jan 28, 2020
@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch 2 times, most recently from 82f5faa to 2d64f44 Compare January 28, 2020 17:49
app/controllers/catalog_controller.rb Outdated Show resolved Hide resolved
@@ -44,10 +44,12 @@
.form-group
.col-md-1{:align => "center"}
#buttons
- template_invalid = Array(@flash_array).any? { |f| f[:level] == :warning }
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not how we want to do it, you have access to the @record, use that instead.

Copy link
Author

@hstastna hstastna Jan 29, 2020

Choose a reason for hiding this comment

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

Yes, thank you, I know. 👍 I was about to change it but you were faster.

:onclick => "#{@row_button[:function]}(\"#{row['id']}\");"
}
# Append a button if @row_button is set and the button is defined in the related decorator
button = item.decorate.try(:gtl_button_cell, row['id'], @row_button[:label], @row_button[:title], @row_button[:function]) if @row_button
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
button = item.decorate.try(:gtl_button_cell, row['id'], @row_button[:label], @row_button[:title], @row_button[:function]) if @row_button
button = item.decorate.try(:gtl_button_cell, row['id'], @row_button.permit(:label, :function, :title)) if @row_button

This way you can access the params in the decorator and decrease the number of arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Even better if we move the constants from @row_button into the decorator and keep it here as a boolean. Let's talk about this when you're at the office.

@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch 4 times, most recently from 2995fd9 to cdd6af0 Compare January 29, 2020 12:15
@hstastna
Copy link
Author

@miq-bot add_label pending core

@hstastna hstastna changed the title [WIP] Identify invalid Catalog Items/Bundles in the UI Identify invalid Catalog Items/Bundles in the UI Jan 29, 2020
@hstastna
Copy link
Author

@miq-bot remove_label wip

@miq-bot miq-bot added pending core and removed wip labels Jan 29, 2020
@row_button = {:label => _("Order"),
:function => "miqOrderService",
:title => _("Order this Service")} # Show a button instead of the checkbox
@row_button = true # Show a button instead of the checkbox
Copy link
Member

Choose a reason for hiding this comment

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

You can make this a one-liner:

Suggested change
@row_button = true # Show a button instead of the checkbox
# Show a button instead of the checkbox
@row_button = true if role_allows?(:feature => 'svc_catalog_provision')

:alt => t = @template_valid ? _("Order this Service") : _("This Service cannot be ordered"),
:title => t,
:disabled => !@template_valid,
:onclick => "miqOrderService(#{@record.id})")
Copy link
Member

Choose a reason for hiding this comment

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

You can reuse the information from the decorator here...

@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch 3 times, most recently from 150ef99 to 7c365b3 Compare January 29, 2020 12:59
@skateman
Copy link
Member

@miq-bot rm_label pending core

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch from 7c365b3 to 7b5746a Compare January 29, 2020 15:19
@mzazrivec mzazrivec closed this Jan 29, 2020
@mzazrivec mzazrivec reopened this Jan 29, 2020
@mzazrivec
Copy link
Contributor

@hstastna The current CI failures seem related.

@hstastna hstastna force-pushed the Identify_invalid_catalog_items branch from 7b5746a to f4d2fb4 Compare January 29, 2020 16:18
@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2020

Checked commits hstastna/manageiq-ui-classic@50d6bdf~...f4d2fb4 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 👍

@mzazrivec mzazrivec self-assigned this Jan 29, 2020
@mzazrivec mzazrivec added this to the Sprint 129 Ending Feb 3, 2020 milestone Jan 29, 2020
@mzazrivec mzazrivec merged commit a64a603 into ManageIQ:master Jan 29, 2020
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.

5 participants