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
Display MIQ/Red Hat Automate domain current/available version in the UI. #5576
Conversation
@h-kataria My understanding was this message would be displayed when you first enter Automate -> Explorer and the root "Datastore" element is selected in the tree and would contain a flash message for each domain that was out-of-sync. With the current approach the user needs to click on the domain before seeing the message. |
version = @record.version | ||
available_version = @record.available_version | ||
return if version.nil? || available_version.nil? | ||
@version_message = _("Currently running %s version, you have %s version available.") % [version, available_version] if version != available_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message should be displayed for all datastores at the same time so it will need to include the domain name. Maybe something like this: "%s domain: Current version - %s, Available version - %s"
Example: ManageIQ domain: Current version - 5.3, Available version - 5.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmcculloug this message in only displayed when user has selected MIQ or Red Hat domain in the tree, message is not displayed on any other node so i am not sure if showing a Domain name is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmcculloug about your other comment about showing this message on Datastore node, i had initially thought that it should only be displayed on Root node of Automate explorer tree, but model methods that are added are instance methods, @mkanoor suggested to add message on Domain level, i can rework these changes if model method can be changed to return version info for all domains in an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be one method that gets passed the domain and checks the version information and returns either nil or the message string. The message that contains the domain name can be used in both places. (No need to maintain two different strings for this.)
Something like this:
def add_all_domains_version_message(domains)
@version_messages = domains.collect do |domain|
domain_version_message(domain)
end.compact
end
def domain_version_message(domain)
version = domain.version
available_version = domain.available_version
return if version.nil? || available_version.nil?
return if version != available_version
_("Currently running %s version, you have %s version available.") % [version, available_version]
end
Then above where you currently call add_domain_version_message
you would do this:
@version_message = domain_version_message(@record) if @record.domain?
@gmcculloug @h-kataria |
a1d56f1
to
2d8150a
Compare
@gmcculloug added messages on Datastore node as well, please review. |
@h-kataria |
@mkanoor i think we can check with UX team after this release to see if they have better ideas of representing such nodes in the tree, rather than adding * or striking out special nodes in the tree, we already have nodes grayed out and striked out and now adding an * in the tree might be too much. |
@grid_data = User.current_tenant.visible_domains | ||
visible_domains = User.current_tenant.visible_domains | ||
@grid_data = visible_domains | ||
add_all_domains_version_message(visible_domains) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visible_domains
variable does not server any purpose here, just pass @grid_data
into the add_all_domains_version_message
method.
2d8150a
to
d52b475
Compare
Added changes to Display MIQ/Red Hat Automate domain current/available versions in the UI if they both don't match. https://bugzilla.redhat.com/show_bug.cgi?id=1283037
d52b475
to
46bfa05
Compare
@mkanoor @gmcculloug addressed your comments, please re-review screenshots: |
👍 |
Checked commit h-kataria@46bfa05 with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0 app/controllers/miq_ae_class_controller.rb
|
@Fryguy This PR is saying that travis is pending but clicking through shows travis completed successfully. Any thoughts? |
@gmcculloug I'm not really sure. If you notice the coverage also fell by a lot, which implies that one of the suites didn't "finish" properly. I'm wondering if we should just kick off the whole thing again. |
Display MIQ/Red Hat Automate domain current/available version in the UI.
Display MIQ/Red Hat Automate domain current/available version in the UI. Added changes to Display MIQ/Red Hat Automate domain current/available versions in the UI if they both don't match. https://bugzilla.redhat.com/show_bug.cgi?id=1283037 (cherry picked from commit 46bfa05) 5.5 BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1287158 clean cherry-pick for ManageIQ#5576 See merge request !564
Added changes to Display MIQ/Red Hat Automate domain current/available versions in the UI if they both don't match.
https://bugzilla.redhat.com/show_bug.cgi?id=1283037
@mkanoor @gmcculloug please review, This PR should be merged after #5548 is merged..