-
Notifications
You must be signed in to change notification settings - Fork 365
Fix start page not being honoured #9520
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
Conversation
Checked commit GilbertCherrie@b4052f8 with ruby 3.1.7, rubocop 1.56.3, haml-lint 0.64.0, and yamllint |
Also, I found issues with some of the shortcuts. These are all fixed in this pr: ManageIQ/manageiq#23515 |
@GilbertCherrie looks good to me. Can you write a test to get the current start page, set the start page, and revert it back to the original start page? I think this fix may improve test speed if we change the default start page to a faster page like utilization. Of course, our tests would need to know how to correctly navigate the page and not rely on login landing you on a specific page. |
@GilbertCherrie If the login redirect hits the API for the /api/users/X for the settings, you might be able to test this by using fixtures to stub the result so you don't need to pollute other tests by setting the start page. In other words, you can fake that response with different values from the API GET to make sure the login direct puts it on the right start page. See #9489 where I give an example of intercept + responding with a fixture. |
7d790e8
to
d3b2c5a
Compare
@asirvadAbrahamVarghese Please also review. |
5fd0f94
to
e533e1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM so far... Just had a minor comment above.
e533e1c
to
d7a410d
Compare
@GilbertCherrie is this ready for re-review? thanks! |
@jrafanie yeah it is |
2088fc7
to
ddc8b59
Compare
ddc8b59
to
de03abd
Compare
de03abd
to
11a9881
Compare
@jrafanie I updated this pr and it should be good now. |
@jrafanie I think this is good to merge now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
import './commands/stub_notifications.js'; | ||
import './commands/throttle_response.js'; | ||
import './commands/toolbar.js'; | ||
import './commands/select.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll sort this list in a followup...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow PR here: #9606
Followup from: ManageIQ#9520
Fixes: #9513
Fixes the issue with the start page in the settings being saved but not actually working when logging into the application.