Selecting Cancel on Settings dashboard give error

[Joseph bajin]

Using the master branch, when I go into the "Settings" dashboard as a user and I click the "cancel" button, I get an error. This happens in the "User Settings" the "Change password" sections. I'm not sure what the correct response should be, should it push the user back the main page, or should it go back to the user settings page which is the default page. If that's the case will the user know what just happened if they clicked on "Cancel" when inside the "User Settings" page.

I get a message (if debug is on) or a 404 page not found that looks like this:

Page not found (404)
Request Method: GET
Request URL: https://XXXXX/settings/None
Using the URLconf defined in openstack_dashboard.urls, Django tried these URL patterns, in this order:
^$ [name='splash']
^api/
^home/$ [name='user_home']
^i18n/js/(?P<packages>\S+?)/$ [name='jsi18n']
^i18n/setlang/$ [name='set_language']
^i18n/
^qunit/$ [name='qunit_tests']
^jasmine/(.*?)$
^admin/
^settings/ ^password/
^settings/ ^$ [name='index']
^identity/
^project/
^auth/
^static\/(?P<path>.*)$
^media\/(?P<path>.*)$
^500/$
The current URL, settings/None, didn't match any of these.
You're seeing this error because you have DEBUG = True in your Django settings file. Change that to False, and Django will display a standard 404 page.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/166569

[Thierry Carrez (ttx)]

Might be included in a RC2 if this is fixed in master soon

[Matt Joyce (matt-nycresistor)]

shipping kilo with a major bug seems like a bad idea. why is this being pushed to liberty-1? there's a proposed fix already in review.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/175702

[Lin Hua Cheng (lin-hua-cheng)]

Proposed a simple long term fix for this bug.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/175702
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=0e116f2a0b35cf4b013ae6fd9b693f096a967e0c
Submitter: Jenkins
Branch: master

commit 0e116f2a0b35cf4b013ae6fd9b693f096a967e0c
Author: lin-hua-cheng <email address hidden>
Date: Mon Apr 20 20:49:31 2015 -0700

    Hide Cancel button if no cancel_url is provided

    If there is no cancel_url is defined on the ModalFormView
    class, hide the Cancel button.

    Change-Id: Id27c60525560dcfdb5cfafa2c4cd7b4edffea158
    Closes-Bug: #1434548

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/175869

[OpenStack Infra (hudson-openstack)]

Change abandoned by David Lyle (<email address hidden>) on branch: master
Review: https://review.openstack.org/166569
Reason: An alternative solution merged based on the review feedback provided in reviews of this patch.

[Joseph bajin (josephbajin)]

I think this was handled really poorly.

As a new contributor, I felt that I was pushed aside and had a different solution committed instead of just telling me what those changes should have been.

Even then, there was no discussion, no review period, nothing. Instead a new change was made and merged, while the patch that has been sitting out for review and discussion completely ignored.

It doesn't make a new contributor feel welcomed in a project. At least it doesn't make me want to contribute very much if this is how my contributions are going to go.

[David Lyle (david-lyle)]

I agree, the flow of this fix was not ideal.

Let's talk about the patch.

On April 15, a core reviewer provided feedback that the cancel button didn't make sense.
On April 16, the PTL provided feedback to remove the cancel button. Additionally commenting on a possible long term fix.
On April 16, a third core reviewer agreed with the feedback re: removing the cancel button.
On April 18, you asked why -1s. The feedback was already provided in the previous reviews.
On April 20, the alternative solution was proposed. The reason for the haste is to include the fix in the Kilo release.

Typically there is more time to fine tune fixes. But if implementing an appropriate fix is relatively simple, there is no reason to commit an inappropriate fix.

I'm happy to work with you on future patches to facilitate a smoother experience.

[Joseph bajin (josephbajin)]

Thank You!

I will definitely take you up on your offer to help.

Even as I re-read those changes, I do not think it was very clear to me
that an alternative solution was necessary. Maybe that's just the way the
team operates and I'm not aware of it yet.

I actually didn't know that the PTL or a core-reviewer had actually
commented. I think that's my own ignorance though.

Thank you again for addressing my concerns and like I mentioned I will take
you up on the help.

On Tue, Apr 21, 2015 at 7:30 PM, David Lyle <email address hidden> wrote:

> I agree, the flow of this fix was not ideal.
>
> Let's talk about the patch.
>
> On April 15, a core reviewer provided feedback that the cancel button
> didn't make sense.
> On April 16, the PTL provided feedback to remove the cancel button.
> Additionally commenting on a possible long term fix.
> On April 16, a third core reviewer agreed with the feedback re: removing
> the cancel button.
> On April 18, you asked why -1s. The feedback was already provided in the
> previous reviews.
> On April 20, the alternative solution was proposed. The reason for the
> haste is to include the fix in the Kilo release.
>
> Typically there is more time to fine tune fixes. But if implementing an
> appropriate fix is relatively simple, there is no reason to commit an
> inappropriate fix.
>
> I'm happy to work with you on future patches to facilitate a smoother
> experience.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1434548
>
> Title:
> Selecting Cancel on Settings dashboard give error
>
> Status in OpenStack Dashboard (Horizon):
> Fix Committed
>
> Bug description:
> Using the master branch, when I go into the "Settings" dashboard as a
> user and I click the "cancel" button, I get an error. This happens in
> the "User Settings" the "Change password" sections. I'm not sure what
> the correct response should be, should it push the user back the main
> page, or should it go back to the user settings page which is the
> default page. If that's the case will the user know what just
> happened if they clicked on "Cancel" when inside the "User Settings"
> page.
>
>
> I get a message (if debug is on) or a 404 page not found that looks like
> this:
>
> Page not found (404)
> Request Method: GET
> Request URL: https://XXXXX/settings/None
> Using the URLconf defined in openstack_dashboard.urls, Django tried
> these URL patterns, in this order:
> ^$ [name='splash']
> ^api/
> ^home/$ [name='user_home']
> ^i18n/js/(?P<packages>\S+?)/$ [name='jsi18n']
> ^i18n/setlang/$ [name='set_language']
> ^i18n/
> ^qunit/$ [name='qunit_tests']
> ^jasmine/(.*?)$
> ^admin/
> ^settings/ ^password/
> ^settings/ ^$ [name='index']
> ^identity/
> ^project/
> ^auth/
> ^static\/(?P<path>.*)$
> ^media\/(?P<path>.*)$
> ^500/$
> The current URL, settings/None, didn't match any of these.
> You're seeing this error because you have DEBUG = True in your Django
> settings file. Change that to False, and Django will display a standard 404
> page.
>
> To manage notifications about this bug go to:
> https://bug...

[Lin Hua Cheng (lin-hua-cheng)]

This is more of an exception...

Folks wanted to get the bug fixed soon to get into Kilo RC, but there is no patch being proposed to address the earlier feedback.

fwiw, there was a review period for the alternative patch. It just got merged fast since folks generally agreed with the alternative - which was the solution as what being suggested earlier in the review.

[Lin Hua Cheng (lin-hua-cheng)]

when in doubt, feel free to jump into #openstack-horizon IRC channel, you can get answers faster.

[Joseph bajin (josephbajin)]

I would agree to a certain point, but at the same time there was no
direction that this patch should not be accepted and that a different way
should be taken. If it was, then it wasn't very clear.

The alternative patch had a review period of about 12 hours. If you go back
and look at when I submitted the patch which was on Mar 21st. I had my two
+1's on the 23rd, and the first patch that questioned anything different
wasn't till April 15th.

I appreciate the response to my concerns, and I just think there should be
a better way of communicating that the solution being proposed is not the
one that should be accepted and to be more direct about the wanted
modifications. I think that would have helped me a lot more than just
waiting around like I did.

On Tue, Apr 21, 2015 at 7:59 PM, Lin Hua Cheng <email address hidden>
wrote:

> This is more of an exception...
>
> Folks wanted to get the bug fixed soon to get into Kilo RC, but there is
> no patch being proposed to address the earlier feedback.
>
> fwiw, there was a review period for the alternative patch. It just got
> merged fast since folks generally agreed with the alternative - which
> was the solution as what being suggested earlier in the review.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1434548
>
> Title:
> Selecting Cancel on Settings dashboard give error
>
> Status in OpenStack Dashboard (Horizon):
> Fix Committed
>
> Bug description:
> Using the master branch, when I go into the "Settings" dashboard as a
> user and I click the "cancel" button, I get an error. This happens in
> the "User Settings" the "Change password" sections. I'm not sure what
> the correct response should be, should it push the user back the main
> page, or should it go back to the user settings page which is the
> default page. If that's the case will the user know what just
> happened if they clicked on "Cancel" when inside the "User Settings"
> page.
>
>
> I get a message (if debug is on) or a 404 page not found that looks like
> this:
>
> Page not found (404)
> Request Method: GET
> Request URL: https://XXXXX/settings/None
> Using the URLconf defined in openstack_dashboard.urls, Django tried
> these URL patterns, in this order:
> ^$ [name='splash']
> ^api/
> ^home/$ [name='user_home']
> ^i18n/js/(?P<packages>\S+?)/$ [name='jsi18n']
> ^i18n/setlang/$ [name='set_language']
> ^i18n/
> ^qunit/$ [name='qunit_tests']
> ^jasmine/(.*?)$
> ^admin/
> ^settings/ ^password/
> ^settings/ ^$ [name='index']
> ^identity/
> ^project/
> ^auth/
> ^static\/(?P<path>.*)$
> ^media\/(?P<path>.*)$
> ^500/$
> The current URL, settings/None, didn't match any of these.
> You're seeing this error because you have DEBUG = True in your Django
> settings file. Change that to False, and Django will display a standard 404
> page.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/horizon/+bug/1434548/+subscriptions
>

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/175869
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=83fcd5a14bf6f8ea8afa19f743044e2c9bccf771
Submitter: Jenkins
Branch: stable/kilo

commit 83fcd5a14bf6f8ea8afa19f743044e2c9bccf771
Author: lin-hua-cheng <email address hidden>
Date: Mon Apr 20 20:49:31 2015 -0700

    Hide Cancel button if no cancel_url is provided

    If there is no cancel_url is defined on the ModalFormView
    class, hide the Cancel button.

    Change-Id: Id27c60525560dcfdb5cfafa2c4cd7b4edffea158
    Closes-Bug: #1434548
    (cherry picked from commit 0e116f2a0b35cf4b013ae6fd9b693f096a967e0c)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/179289

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/179289
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=aba74d04a530fb6c251c15037da96b7a93b83a7e
Submitter: Jenkins
Branch: master

commit 56d5683c8c49558ae22200bc01ec87ea83144003
Author: Akihiro Motoki <email address hidden>
Date: Thu Apr 23 06:58:49 2015 +0900

    Import translations from Transifex for Juno

    * Import ~100% completed translations
      (translations available for 12 languages)
    * Delete incomplete languages
    * Update language list in openstack_dashboard settings.py

    Change-Id: Ie65227e510d95504ed94fb35f0f7273739759601

commit 89d9f46750a7506d6910ae792c14ad47feaf53c3
Author: Doug Fish <email address hidden>
Date: Tue Apr 14 15:05:41 2015 -0500

    Format numbers in a locale sensitive manner

    Create Volume, Create Snapshot, Extend Volume and
    Flavors and Quota all fail to format numbers in a
    locale sensitive manner. This patch changes the code to
    use the intcomma filter before quota, instead of after. Once
    quota has added text the number can no longer be formatted.

    (Pulled from gate, can't pass checks)

    Change-Id: Id9749d9b25d93185b917708b85471e6f765033e2
    Closes-Bug: 1314801
    (cherry picked from commit 0872b4a40334a6df863f88e5aa56b31363aff28e)

commit a1ac5936211cc134a6fd5246007889a5149a18dd
Author: Akihiro Motoki <email address hidden>
Date: Sat Apr 18 19:53:06 2015 +0900

    update .tx/config to match Kilo Transifex resources

    (Pulled from gate, can't pass checks)

    Change-Id: I167d7787d3096c35a9ad73634e69058306731e44

commit 2e1216136a93a1c2129d9de05fcbbd032fae76e2
Author: OpenStack Proposal Bot <email address hidden>
Date: Thu Apr 23 15:48:09 2015 +0000

    Updated from global requirements

    Change-Id: I5602a6489684db95934130f3ce83c8eb37af2d3d

commit 624a85c7eee4e4fb4f88ccc8bd52caa6c0423e89
Author: lin-hua-cheng <email address hidden>
Date: Wed Apr 22 14:45:26 2015 -0700

    Initialize variable 'networks' before referencing

    Fixes the possible UnboundLocalError on the utility class.

    Change-Id: I52d27f90a583f1216a927147d1bd414c5f866608
    Closes-Bug: #1444421
    (cherry picked from commit 2e4996d2d220df0ae36a863fe694706e6f7aeb40)

commit 5c1604275bfbe621e5e168f8ba1f937d308d6ab8
Author: Janet Yu <email address hidden>
Date: Fri Mar 13 13:50:42 2015 -0700

    Fix addition of plugin panel to panel group

    Revert fix from bug #1329050, which adds a plugin panel to its dashboard's
    class. Adding a plugin panel to a dashboard whose class has its panels defined
    in a tuple will fail because the new plugin cannot be appended to the tuple.
    The code errors out before the panel gets added to the dashboard's panel list.
    However, at this point, the panel has already been registered with the
    dashboard. This causes Dashboard.get_panels() to see the panel in its registry
    but not in its panel groups. Consequently, the panel gets put in the "Other"
    panel group instead of its configured one, as described in bug #1378558.

    By not adding the the plugin panel to its dashboard's class, the panel gets
    a...