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

Fix tags format #1180

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Fix tags format #1180

merged 1 commit into from
Apr 27, 2017

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Apr 27, 2017

Description

Description of problem:
Smart management tags are not show on current format on the following views:

  1. Routes
  2. Container Services
  3. Nodes
  4. Container Templates
  5. Image Registries

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1439776

Screenshoots

screenshot-localhost 3000-2017-04-27-17-06-02
screenshot-localhost 3000-2017-04-27-17-07-57
screenshot-localhost 3000-2017-04-27-17-08-49
screenshot-localhost 3000-2017-04-27-17-14-49
screenshot-localhost 3000-2017-04-27-17-15-47

@yaacov
Copy link
Member Author

yaacov commented Apr 27, 2017

@miq-bot add_label compute/containers, bug

@simon3z @zeari @dkorn @moolitayer @himdel please review

@simon3z
Copy link
Contributor

simon3z commented Apr 27, 2017

@yaacov when/where this regression was introduced?

@moolitayer
Copy link

cc @martinpovolny

@yaacov
Copy link
Member Author

yaacov commented Apr 27, 2017

@yaacov when/where this regression was introduced?

@simon3z
This are leftovers from a big re-factor 3 months ago:
#274

@simon3z
Copy link
Contributor

simon3z commented Apr 27, 2017

LGTM 👍
@miq-bot assign himdel
cc @martinpovolny @h-kataria @dclarizio

@simon3z
Copy link
Contributor

simon3z commented Apr 27, 2017

@miq-bot add_label fine/yes

@himdel
Copy link
Contributor

himdel commented Apr 27, 2017

@yaacov I think you've missed a few...

app/helpers/container_helper/textual_summary.rb
app/helpers/container_project_helper/textual_summary.rb
app/helpers/persistent_volume_helper/textual_summary.rb
app/helpers/container_image_helper/textual_summary.rb
app/helpers/container_group_helper/textual_summary.rb
app/helpers/container_build_helper/textual_summary.rb
app/helpers/container_replicator_helper/textual_summary.rb

(all of those have a textual_group_smart_management that does the items.collect thing)

Otherwise, LGTM 👍

@yaacov
Copy link
Member Author

yaacov commented Apr 27, 2017

@yaacov I think you've missed a few...

Woooo 👍 , they have a different bug :-) they use TextualTags and not TextualGroup like the ones in the BZ. Here we get "No My Company Tags have been assigned" message , when you do have one :-(

Fixing it now :-)

[EDIT]
No, it was me not setting the tags correctly .... , they just work correctly because they use TextualTags and not TextualGroup

@yaacov
Copy link
Member Author

yaacov commented Apr 27, 2017

@himdel 👍 added the missing files [ they did not show the bug because they where using TextualTags and not TextualGroup ] but the code is nicer now :-)

@dclarizio
Copy link

@yaacov

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1444636

This BZ is closed, can you change to the one referenced as the dup? Thx, Dan

@yaacov
Copy link
Member Author

yaacov commented Apr 27, 2017

This BZ is closed, can you change to the one referenced as the dup? Thx, Dan

@dclarizio 👍 changed in description to https://bugzilla.redhat.com/show_bug.cgi?id=1439776

those have a textual_group_smart_management that does the items.collect thing

@moolitayer, @himdel , looking at it again, the problem was not the items.collect but the TextualGroup, I think we need the call for send("textual_#{m}") to hide some tags that user should not see @dkorn ?

Updated screenshots in description, using new code, it fixes the bug, but does not change the logic of the function.

@himdel
Copy link
Contributor

himdel commented Apr 27, 2017

@yaacov nope, it was better as you had it before .. everywhere else uses just TextualTags.new(_("Smart Management"), %i(tags)) or sometimes TextualTags.new(_("Smart Management"), %i(zone tags)).

The code is semantically equivalent (see expand_textual_summary in textual_summary_helper.rb) - when it gets a symbol, it essentially does the same thing (except for a few additional fixups which is why we prefer the symbol version).

@yaacov
Copy link
Member Author

yaacov commented Apr 27, 2017

@himdel 👍 done

@miq-bot
Copy link
Member

miq-bot commented Apr 27, 2017

Checked commit yaacov@b1c3064 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
12 files checked, 0 offenses detected
Everything looks fine. 🍪

@himdel
Copy link
Contributor

himdel commented Apr 27, 2017

Thanks, LGTM, waiting for travis.. :)

@martinpovolny martinpovolny merged commit cadbf73 into ManageIQ:master Apr 27, 2017
@martinpovolny martinpovolny added this to the Sprint 60 Ending May 8, 2017 milestone Apr 27, 2017
@chrispy1
Copy link

@miq-bot add_label blocker

simaishi pushed a commit that referenced this pull request Apr 28, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 7e8cf4b0bb7d34372722c559416fbcafd41659d2
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Thu Apr 27 20:09:43 2017 +0200

    Merge pull request #1180 from yaacov/tags-correct-format
    
    Fix tags format
    (cherry picked from commit cadbf73337ecfcef2c4a6f2625795480626e7d4e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1446775

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.

9 participants