Refs #22049 - improve metadata usability #5378

ares · 2018-03-29T13:26:13Z

This is a follow up of #5359

before

after

@tbrisker and @Rohoover, your comments would be appreciated, I'd like to get this into 1.18

theforeman-bot · 2018-03-29T13:26:23Z

Issues: #22049

tbrisker

tbh, i'm not sure I understand the workflow this is meant to support.
When viewing an audit, why would the next step be editing one of the location affected or the user who made the change? It might make some sense if we had show pages for those objects that provide further information, but i am not sure how much this is needed, and it significantly increases the code complexity.

ares · 2018-04-01T09:57:56Z

As a User, if I want to see information about org/loc in which the event happend I only can see that on edit page. If we had show pages, I'd link to those. It's simply "give me more information" about the resource. But at least from edit page, once can find out what other resources are assigned.

I don't understand the code complexity concern. The link construction is the same as with any other link. Methods for generating all links contain if condition to nicely display the information, that there's no org/loc to be displayed, instead of white space. IMHO better user experience.

tbrisker

Ok, I guess this is an overall improvement. A few minor comments inline (same apply to the second function as well).

tbrisker · 2018-04-01T10:57:57Z

app/helpers/audits_helper.rb

+    base = audit.locations.authorized(:view_locations)
+    authorizer = Authorizer.new(User.current, base)
+    if base.empty?
+      _('none')


This may also be "all" if the auditable isn't taxable or has taxonomies, we might want to also accommodate that case?

also, you can return _('none') if base.empty? before you build the authorizer and reduce the nesting and unneeded creation.

tbrisker · 2018-04-02T13:47:45Z

app/helpers/audits_helper.rb

+    if base.empty?
+      _('none')
+    else
+      base.map { |location| link_to_if_authorized location.name, hash_for_edit_location_path(location).merge(:auth_object => location, :permission => 'edit_locations', :authorizer => authorizer) }.to_sentence.html_safe


This is a very long line, maybe break it into a do...end block so it is easier to read?

houndci-bot

Some files could not be reviewed due to errors:

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `S...

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Performance/HashEachMethods` cop has been removed since it no longer provides performance benefits in modern rubies.

ares · 2018-04-03T11:52:16Z

Thanks @tbrisker, all comments addressed. we display all now in case Taxonomix is not included in the audited resource. I also added missing org/loc association into API template, that should have been added when we started to display the metadata. Could you please take another look?

houndci-bot

Some files could not be reviewed due to errors:

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `S...

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Performance/HashEachMethods` cop has been removed since it no longer provides performance benefits in modern rubies.

houndci-bot

Some files could not be reviewed due to errors:

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `S...

Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
The `Performance/HashEachMethods` cop has been removed since it no longer provides performance benefits in modern rubies.

ares · 2018-04-03T12:00:15Z

repushed with extracted "all" check into separate method

tbrisker · 2018-04-03T13:40:11Z

app/helpers/audits_helper.rb

+
+  def affected_taxonomies(audit)
+    begin
+      return _('all') unless audit.auditable_type.constantize.included_modules.include?(Taxonomix)


This will return all for hosts and taxonomies, since they don't include Taxonomix, and might be true for other resources as well that have different ways of linking to taxonomies

is there a better way to detect on your mind? I think it's be better if we don't display all at all, there's no proper link between audit and taxonomies in this case and it might be confusing if people search for audits using organization attribute, they would expect to see it even if they search for organization = Default but the audit only appears if we search for null? organization

tbrisker · 2018-04-03T13:40:31Z

app/helpers/audits_helper.rb

+  def affected_taxonomies(audit)
+    begin
+      return _('all') unless audit.auditable_type.constantize.included_modules.include?(Taxonomix)
+    rescue => e


When might we get an exception here?

if we have audit for resource that no longer exists (plugin uninstallation, refactoring, etc), we can't rely on constantize to succeed

tbrisker · 2018-04-03T13:44:39Z

app/views/audits/show.html.erb

      </div>
      <div class="row">
        <strong class="col-md-2"><%= _("User Name:")%></strong>
-        <div class="col-md-10"><%= @audit.username %></div>
+        <div class="col-md-10"><%= audit_user(@audit) %></div>


Note that this leads to all audits by this user, not the user itself, not sure if this was intentional or not

it is, because it's the same thing we link on audits list, I though it would be better to be consistent and change the helper when we improve the UI

tbrisker · 2018-04-03T13:46:49Z

app/helpers/audits_helper.rb

+    end.to_sentence.html_safe
+  end
+
+  def affected_taxonomies(audit)


the function name is misleading

please suggest better one :-) how about "all_affected_taxonomies_string"?

tbrisker

Thanks @ares !

Rohoover · 2018-04-09T18:28:32Z

LGTM.

theforeman-bot added Needs testing Not yet reviewed labels Mar 29, 2018

tbrisker reviewed Apr 1, 2018

View reviewed changes

tbrisker suggested changes Apr 2, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Apr 2, 2018

ares force-pushed the refs/22049 branch from 8d3de91 to cb3a6dc Compare April 3, 2018 11:50

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Apr 3, 2018

houndci-bot reviewed Apr 3, 2018

View reviewed changes

ares force-pushed the refs/22049 branch from cb3a6dc to 30d9135 Compare April 3, 2018 11:53

houndci-bot reviewed Apr 3, 2018

View reviewed changes

ares force-pushed the refs/22049 branch from 30d9135 to a4d2fdd Compare April 3, 2018 11:59

houndci-bot reviewed Apr 3, 2018

View reviewed changes

tbrisker suggested changes Apr 3, 2018

View reviewed changes

Refs #22049 - improve metadata usability

d93703c

ares force-pushed the refs/22049 branch from a4d2fdd to d93703c Compare April 4, 2018 10:53

tbrisker approved these changes Apr 4, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 4, 2018

tbrisker added Ready to merge and removed Needs testing labels Apr 4, 2018

tbrisker added this to the 1.18.0 milestone Apr 4, 2018

tbrisker merged commit af09d63 into theforeman:develop Apr 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #22049 - improve metadata usability #5378

Refs #22049 - improve metadata usability #5378

ares commented Mar 29, 2018

theforeman-bot commented Mar 29, 2018

tbrisker left a comment

ares commented Apr 1, 2018

tbrisker left a comment

tbrisker Apr 1, 2018

tbrisker Apr 2, 2018

tbrisker Apr 2, 2018

houndci-bot left a comment

ares commented Apr 3, 2018

houndci-bot left a comment

houndci-bot left a comment

ares commented Apr 3, 2018

tbrisker Apr 3, 2018

ares Apr 3, 2018

tbrisker Apr 3, 2018

ares Apr 3, 2018

tbrisker Apr 3, 2018

ares Apr 3, 2018

tbrisker Apr 3, 2018

ares Apr 3, 2018

tbrisker left a comment

Rohoover commented Apr 9, 2018

Refs #22049 - improve metadata usability #5378

Refs #22049 - improve metadata usability #5378

Conversation

ares commented Mar 29, 2018

theforeman-bot commented Mar 29, 2018

tbrisker left a comment

Choose a reason for hiding this comment

ares commented Apr 1, 2018

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

ares commented Apr 3, 2018

houndci-bot left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

ares commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment

Rohoover commented Apr 9, 2018