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

user_input_filter - don't send Cancel on Apply #5875

Merged
merged 1 commit into from Jul 29, 2019
Merged

user_input_filter - don't send Cancel on Apply #5875

merged 1 commit into from Jul 29, 2019

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jul 25, 2019

Go to Advanced search in an explorer controller (Compute > Infrastructure > VMs, accordion VMs),
add a filter which uses user input (say, VM power state == user input),
apply the filter,
(watch the Network tab,)
click Apply on the user input modal (after filling something in).

Before: you'll see requests to both quick_search?button=apply and quick_search?button=cancel.
After: no cancel, just apply


Since #3444 (which fixed the user input filter modal to hide after Apply, closing #499):

  • clicking the apply button triggers a ?button=apply request
  • but, thanks to data-dismiss="modal", it also closes the modal
  • BUT, closing the modal this way triggers a ?button=cancel request

Thus, clicking apply triggers apply & cancel, in random order.
(Which sometimes works and sometimes doesn't.)


Ideally, we should not be using data-dismiss on the Apply button,
but using $('#quicksearchbox').modal('hide') in quick_search_apply_click, same as in quick_search_cancel_click.

The problem with that is that this would involve changing every single replace_right_cell, or somehow amending their output after the fact.

So, leaving the data-dismiss mechanism as is, but:

  • explicitly adding code to not send the ?button=cancel request when the Apply button was clicked
  • explicitly triggering hiding the modal when enter was clicked in the form (previously it would just apply)

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

Cc @hstastna

Since #3444 (which fixed the user input filter modal to hide after Apply):

* clicking the apply button triggers a ?button=apply request
* but, thanks to data-dismiss="modal", it also closes the modal
* BUT, closing the modal this way triggers a ?button=cancel request

Thus, clicking apply triggers apply & cancel, in random order.

Ideally, we should not be using data-dismiss on the Apply button,
but using `$('#quicksearchbox').modal('hide')` in `quick_search_apply_click`, same as in `quick_search_cancel_click`.

The problem with that is that this would involve changing every single `replace_right_cell`, or somehow amending their output after the fact.

So, leaving the data-dismiss mechanism as is, but:

* explicitly adding code to not send the ?button=cancel request when the Apply button was clicked
* explicitly triggering hiding the modal when enter was clicked in the form (previously it would just apply)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1715550
@miq-bot
Copy link
Member

miq-bot commented Jul 25, 2019

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/d25f5ed965d7f75b7f7a094bde4dee564e8258c0 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Contributor

@hstastna hstastna left a comment

Choose a reason for hiding this comment

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

When I try this scenario, problems occur:

  1. Compute > Infra > VMs > VMs accordion
  2. Open Adv search, create filter: choose something where it is possible to choose between true/false values ("user will input the value")
  3. Click on Apply and choose something from the drop down
    => there is still <Choose> 'value' in the drop down :(

If I try to apply the filter, something happens, filter is applied but not sure which value was chosen.
And yes, just button=apply was there, this was fixed 👍 I like your solution.

Anyway, I am able to reproduce this issue on your branch but also on master. Not sure if it is related.

@himdel
Copy link
Contributor Author

himdel commented Jul 29, 2019

Anyway, I am able to reproduce this issue on your branch but also on master

Thanks! But leaving for another PR, as that sounds like something specific to booleans vs user input.

(created #5896 for now)

@mzazrivec mzazrivec self-assigned this Jul 29, 2019
@mzazrivec mzazrivec added this to the Sprint 117 Ending Aug 5, 2019 milestone Jul 29, 2019
@mzazrivec mzazrivec merged commit b9568e2 into ManageIQ:master Jul 29, 2019
@himdel himdel deleted the adv-apply branch July 29, 2019 14:57
simaishi pushed a commit that referenced this pull request Jul 29, 2019
user_input_filter - don't send Cancel on Apply

(cherry picked from commit b9568e2)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1734122
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 02c3dbff9f6d33adff31fa6136ae3235bb6bf681
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Jul 29 16:56:53 2019 +0200

    Merge pull request #5875 from himdel/adv-apply
    
    user_input_filter - don't send Cancel on Apply
    
    (cherry picked from commit b9568e2f734a510eaddd4167d6729e114df92aea)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1734122

simaishi pushed a commit that referenced this pull request Jul 29, 2019
user_input_filter - don't send Cancel on Apply

(cherry picked from commit b9568e2)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1715550
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 3dbfd78b22dac33bd33374afa0b55fa3f3d33df3
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Jul 29 16:56:53 2019 +0200

    Merge pull request #5875 from himdel/adv-apply
    
    user_input_filter - don't send Cancel on Apply
    
    (cherry picked from commit b9568e2f734a510eaddd4167d6729e114df92aea)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1715550

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.

None yet

5 participants