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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Catalog Item with Tag Control element cannot be ordered #278

Merged
merged 1 commit into from Apr 16, 2018

Conversation

Projects
None yet
4 participants
@eclarizio
Member

eclarizio commented Apr 13, 2018

Before, we were using lodash's _.isEmpty method to determine if something was selected or not, but _.isEmpty does not behave the Ruby version we know and love. It only determines if an object is empty, not a variable. So, the problem was that the tag control select was passing in a category id, which was a number, and _.isEmpty(1234) returns true. Therefore validation was failing 馃槩.

This changes the logic so that when a single value is to be selected, we check to see if it's a number. If it's 0 (which is the "choose" value), then we still need to select something, otherwise we should be ok. If it somehow gets set to null, we should still also be failing validation.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1567108

@miq-bot add_label gaprindashvili/yes, bug

@miq-bot assign @himdel

@himdel It's not currently marked as a blocker but will likely be a blocker for 5.9.3, just so you know.
@chalettu Can you review please?

/cc @gmcculloug

@miq-bot

This comment has been minimized.

Member

miq-bot commented Apr 13, 2018

Checked commit eclarizio@4c9b2ac with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 馃嵃

@chalettu

I think this all looks solid. I would change things just a little to be a little clearer by removing the empty check from forceSingleValue and just making it an if statement below

if (fieldValue === 0) {
invalid = true;
}
} else if (_.isEmpty(fieldValue)) {

This comment has been minimized.

@chalettu

chalettu Apr 13, 2018

Contributor

I think this can be made a little more concise by removing it from here and removing the else below and just changing the else to an if.

} else if (_.isEmpty(fieldValue)) {
invalid = true;
}
} else {

This comment has been minimized.

@chalettu

chalettu Apr 13, 2018

Contributor

Remove the else but keep the if statement that is inside this. Same logic, just cuts down on a few lines

This comment has been minimized.

@eclarizio

eclarizio Apr 13, 2018

Member

It's not the same logic though, because _.isEmpty will return true when the value that gets passed in is a number, which is wrong.

This comment has been minimized.

@chalettu

chalettu Apr 16, 2018

Contributor

Ah, gotcha. Sorry I misinterpreted it. Looks good to me.

@himdel

This comment has been minimized.

Contributor

himdel commented Apr 16, 2018

@eclarizio So, the default "bad" branch used to be fieldValue === "" before it got changed to _.isEmpty(fieldValue) in #204.

If isEmpty is always wrong for non-objects, shouldn't we also change the else condition to trigger on !fieldValue || (_.isObject(fieldValue) && _.isEmpty(fieldValue))?

(The PR looks good as is, but I'm wondering if #204 is not introducing more than just this bug because of it.)

@eclarizio

This comment has been minimized.

Member

eclarizio commented Apr 16, 2018

@himdel Good point, I think the other scenario where it could fail then is when the field is required and the data_type of the field is set to integer, because then _.isEmpty would return true. I'll test today and make sure, but you're right in that I don't know if we want to hold up this PR for that.

@himdel

This comment has been minimized.

Contributor

himdel commented Apr 16, 2018

I'll test today and make sure, but you're right in that I don't know if we want to hold up this PR for that.

@eclarizio As long as you're on it :).

OK, so looks like we can merge? :)

@eclarizio

This comment has been minimized.

Member

eclarizio commented Apr 16, 2018

I think so. @chalettu You good with this?

@himdel himdel merged commit c1319ef into ManageIQ:master Apr 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.4%) to 70.739%
Details

@himdel himdel added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 16, 2018

@himdel

This comment has been minimized.

Contributor

himdel commented Apr 16, 2018

Released in 1.1.11.

Backported to gaprindashvili:

commit d038b37063b03cf7bd2d1c5a10cc1b19b3fb7c14 (HEAD -> gaprindashvili)
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Apr 16 17:15:40 2018 +0200

    Merge pull request #278 from eclarizio/BZ1567108
    
    Fix for Catalog Item with Tag Control element cannot be ordered
    
    (cherry picked from commit c1319ef1a90f6e9cfc4dc093d6e62f94aad77a23)

Released in 1.0.22.

himdel added a commit that referenced this pull request Apr 16, 2018

Merge pull request #278 from eclarizio/BZ1567108
Fix for Catalog Item with Tag Control element cannot be ordered

(cherry picked from commit c1319ef)
@chalettu

This comment has been minimized.

Contributor

chalettu commented Apr 16, 2018

@eclarizio , Yes. Totally fine. Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment