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

Redirecting to 'show list' after deleting a 'cloud network' #1809

Merged
merged 1 commit into from
Sep 11, 2017

Conversation

AlonaKaplan
Copy link
Contributor

Currently it stays in the deleted network form. It is not standard. For example, when 'cloud subnet' is deleted it is redirected to the subnets list.

@AlonaKaplan
Copy link
Contributor Author

please review @tzumainn @Ladas

@tzumainn
Copy link
Contributor

tzumainn commented Aug 2, 2017

@gildub sorry, this one too!

@Ladas
Copy link
Contributor

Ladas commented Aug 4, 2017

@himdel can you point us to what is the right way? The flash message passing should be automatic?

@himdel
Copy link
Contributor

himdel commented Aug 11, 2017

Well, this works, but yeah we shouldn't be passing these flash messages as params if we can help it.

For example, AnsibleCredentialController and AnsibleRepositoryController do...

    session[:flash_msgs] = @flash_array
    javascript_redirect :action => 'show_list'

Similarly, CloudVolumeController, CloudSubnetController and NetworkRouterController do...

      drop_breadcrumb(:name => 'dummy', :url => " ") # missing a bc to get correctly back so here's a dummy
      session[:flash_msgs] = @flash_array.dup if @flash_array
      redirect_to(previous_breadcrumb_url)

👉 I guess it'd be best to follow GenericFormMixin though, which should be the most current and generic...

        session[:flash_msgs] = @flash_array.dup if @flash_array
        javascript_redirect(previous_breadcrumb_url)

(feel free to use :action => 'show_list' if previous_breadcrumb_url proves impossible to use, but prefer the latter, since you can come to most details screens from multiple sources (list of all networks vs. a list of networks under a provider, etc.))

But, if we're standardizing the delete behaviour, we should probably fix all of them:

  • CloudVolumeSnapshotController just calls render_flash same as here before this PR

  • these call javascript_redirect with the first flash message, same as here now:

    • SecurityGroupController
    • CloudTenantController
    • FloatingIpController

@gildub
Copy link
Contributor

gildub commented Aug 22, 2017

@tzumainn, catching up too...

@gildub
Copy link
Contributor

gildub commented Aug 23, 2017

This is definitely needed.

@himdel himdel self-assigned this Aug 23, 2017
@himdel
Copy link
Contributor

himdel commented Aug 23, 2017

@AlonaKaplan just a heads up (no pressure), this is now waiting for you :)

@miq-bot
Copy link
Member

miq-bot commented Aug 29, 2017

Checked commit AlonaKaplan@ebe330a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@himdel himdel added the wip label Aug 30, 2017
@himdel himdel changed the title Redirecting to 'show list' after deleting a 'cloud network' [WIP] Redirecting to 'show list' after deleting a 'cloud network' Aug 30, 2017
@AlonaKaplan
Copy link
Contributor Author

@dankenigsberg

@himdel himdel added bug and removed wip labels Sep 11, 2017
@himdel himdel changed the title [WIP] Redirecting to 'show list' after deleting a 'cloud network' Redirecting to 'show list' after deleting a 'cloud network' Sep 11, 2017
@himdel himdel added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 11, 2017
@himdel
Copy link
Contributor

himdel commented Sep 11, 2017

LGTM, tested in the UI :)

@himdel himdel merged commit eb5a9dc into ManageIQ:master Sep 11, 2017
@alexander-demicev
Copy link

simaishi pushed a commit that referenced this pull request Nov 13, 2017
Redirecting to 'show list' after deleting a 'cloud network'
(cherry picked from commit eb5a9dc)

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

Fine backport details:

$ git log -1
commit 5ccd31e1b2033f5cb2fa6a7461b65d69ad78d342
Author: Martin Hradil <[email protected]>
Date:   Mon Sep 11 15:02:08 2017 +0000

    Merge pull request #1809 from AlonaKaplan/del_net
    
    Redirecting to 'show list' after deleting a 'cloud network'
    (cherry picked from commit eb5a9dce497a338b0efddc467e8f09ebd3959df8)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1512667

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.

8 participants