Fixes #37013 - change the 'All hosts' menu item's url based on the `new_host_page` setting #9969

Ron-Lavi · 2023-12-28T12:38:31Z

After changing the new_host_page setting, the layout navigation still has the wrong hosts page link (the one it got initially)
and someone who wants to try this new functionallity can't access the page via the navigation.

Using ForemanContext, in this PR we are updating the menu item based on the changed setting.

MariaAga · 2024-01-02T11:42:35Z

This (and lab_features ,instance_title) settings say that they require refresh.
We can force a reload of the page when a user changes one of these settings specifically.
I do think that the best thing to do is just leave it as is, so users will refresh when they want to.

Ron-Lavi · 2024-01-03T08:05:25Z

I think we could store those settings in a constant, e.g:
SETTINGS_REQUIRE_REFRESH = ['new_hosts_page', 'lab_features' , 'instance_title']

and check on successful setting update if it's one of those three to trigger refresh.

The refresh itself is pretty quick and increases the user experience for using those settings.

I was also a bit surprised when trying to use the new hosts page, and when going to that page I saw that nothing have changed.

In addition, when you are already in the new hosts page, and trying to reload the page, you will not move to the new hosts page because the URL itself belong to the legacy hosts page.

@MariaAga what do you think?

ekohl · 2024-01-03T12:08:48Z

Automatically refreshing a page can be bad. Also, if a user has multiple tabs open then they won't get it there. The experience may be inconsistent. The tab may even be on a different device or different user so you can't refresh those.

Like @MariaAga I think we shouldn't refresh automatically, but make the user aware of it. Showing a warning after can be a good thing.

jeremylenz · 2024-01-03T15:35:54Z

@Ron-Lavi We discussed this at our meeting and the outcome was

The "right" way to fix this would be to update the links in React. This prevents a browser page reload (and as far as the page being open in other browser tabs, we don't solve for that anywhere else so that's not a big concern.)
However, we know it might be a lot of work to do the above. So if it's too much, for now let's just "fix it with words" -
- Update the wording to "(requires Settings page reload)" so that users don't immediately click All Hosts and go to the wrong page
- Change the "(requires Settings page reload)" to be in the setting name and not the description, so it's more prominent.
Before implementing anything, consult with the original BZ reporter to validate that they will accept the solution.

@MariaAga @ares @MariSvirik if I missed anything, please comment here

Ron-Lavi · 2024-01-09T08:15:03Z

Thanks @jeremylenz and the team!
I think I will go with a ForemanContext update and deal with the menu link update.

ekohl · 2024-01-09T10:26:42Z

@Ron-Lavi remember that the problem will still exist in other tabs or browsers of other users. AFAIK we don't have a mechanism to push a refresh or poll for new URLs.

In the past we've talked about implementing ActionCable (also to remove polling for user notifications). Back then we were blocked on other issues in the stack, but those have long been resolved. Perhaps it makes sense to implement it as a more generic subscribe mechanism where notifications are one, but some broadcast about settings/menu links is another?

You can argue about whether it's surprising the user (=bad) if their UI suddenly changes though.

Ron-Lavi · 2024-01-09T20:09:52Z

In the past we've talked about implementing ActionCable (also to remove polling for user notifications). Back then we were blocked on other issues in the stack, but those have long been resolved. Perhaps it makes sense to implement it as a more generic subscribe mechanism where notifications are one, but some broadcast about settings/menu links is another?

that will be great for many areas in our app indeed.

You can argue about whether it's surprising the user (=bad) if their UI suddenly changes though.

I think it's fine in case we are making things more accurate

@Ron-Lavi remember that the problem will still exist in other tabs or browsers of other users.

Agree.. probably ActionCable is the way to go, but as for now we don't support those changes across multiple tabs unfortunately.

Ron-Lavi · 2024-01-10T08:43:25Z

Ready for another review round @MariaAga @jeremylenz :)

jeremylenz

Looks like the plugin tests are still failing on this:

TypeError: Cannot read property 'context' of undefined
          at useForemanContext (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/Root/Context/ForemanContext.js:7:40)
          at useForemanMetadata (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/Root/Context/ForemanContext.js:11:34)
          at useForemanSettings (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/Root/Context/ForemanContext.js:14:41)
          at Pagination (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/components/Pagination/index.js:32:63)
          at renderWithHooks (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:14803:18)
          at mountIndeterminateComponent (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:17482:13)
          at beginWork (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:18596:16)
          at HTMLUnknownElement.callCallback (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:188:14)
          at invokeEventListeners (/home/runner/work/foreman/foreman/projects/plugin/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:193:27)
          at HTMLUnknownElementImpl._dispatch (/home/runner/work/foreman/foreman/projects/plugin/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
          at HTMLUnknownElementImpl.dispatchEvent (/home/runner/work/foreman/foreman/projects/plugin/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
          at HTMLUnknownElementImpl.dispatchEvent (/home/runner/work/foreman/foreman/projects/plugin/node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
          at HTMLUnknownElement.dispatchEvent (/home/runner/work/foreman/foreman/projects/plugin/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
          at Object.invokeGuardedCallbackDev (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:237:16)
          at invokeGuardedCallback (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:292:31)
          at beginWork$1 (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:23203:7)
          at performUnitOfWork (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:22157:12)
          at workLoopSync (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:22130:22)
          at performSyncWorkOnRoot (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:21756:9)
          at /home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:11089:24
          at unstable_runWithPriority (/home/runner/work/foreman/foreman/projects/foreman/node_modules/scheduler/cjs/scheduler.development.js:[653](https://github.com/theforeman/foreman/actions/runs/7472314844/job/20334243456?pr=9969#step:10:654):12)
          at runWithPriority$1 (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:11039:10)
          at flushSyncCallbackQueueImpl (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:11084:7)
          at flushSyncCallbackQueue (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:11072:3)
          at batchedUpdates$1 (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-dom/cjs/react-dom.development.js:21862:7)
          at Object.notify (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-redux/lib/utils/Subscription.js:21:7)
          at Object.notifyNestedSubs (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-redux/lib/utils/Subscription.js:92:15)
          at handleChangeWrapper (/home/runner/work/foreman/foreman/projects/foreman/node_modules/react-redux/lib/utils/Subscription.js:97:20)
          at next (/home/runner/work/foreman/foreman/projects/foreman/node_modules/redux/lib/redux.js:305:7)
          at /home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIMiddleware.js:13:10
          at /home/runner/work/foreman/foreman/projects/foreman/node_modules/redux-thunk/lib/index.js:27:16
          at dispatch (/home/runner/work/foreman/foreman/projects/foreman/node_modules/redux/lib/redux.js:699:28)
          at call (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIRequest.js:48:5)
          at tryCatch (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIRequest.js:2:1)
          at Generator._invoke (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIRequest.js:2:1)
          at Generator.next (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIRequest.js:2:1)
          at asyncGeneratorStep (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIRequest.js:2:1)
          at _next (/home/runner/work/foreman/foreman/projects/foreman/webpack/assets/javascripts/react_app/redux/API/APIRequest.js:2:1)
          at processTicksAndRejections (internal/process/task_queues.js:95:5)
          at runNextTicks (internal/process/task_queues.js:64:3)
          at processImmediate (internal/timers.js:437:9)

Ron-Lavi · 2024-01-15T10:35:37Z

Looks like last commit fixed the failing Katello JS tests @jeremylenz

MariaAga · 2024-01-15T13:14:41Z

@Ron-Lavi did you get to Before implementing anything, consult with the original BZ reporter to validate that they will accept the solution. ? (I didnt see it in the BZ itself)

Ron-Lavi · 2024-01-16T09:24:31Z

Hey @MariaAga, yes, verified with Jayant and it works for him.

webpack/assets/javascripts/react_app/Root/ReactApp.js

MariaAga · 2024-01-16T10:28:16Z

Should we remove the (requires reload of page) text from the setting?

MariaAga · 2024-01-16T13:51:33Z

Looks ok to me, @jeremylenz any requests?

ianballou · 2024-01-16T14:56:20Z

If that VCR test failure on Katello passes locally, let's ignore the error. It believe it's a super rare transient failure that I haven't seen in a long while.

jeremylenz · 2024-01-16T15:07:46Z

@MariaAga Just tested again and continues to work well, LGTM!

@Ron-Lavi please double-check Ian's comment above and also please squash the commits, that's required in Foreman nowadays :) After that it should be ready to merge.

…ew_host_page` setting After changing the `new_host_page` setting, the layout navigation still has the wrong hosts page link (the one it got initially) and someone who wants to try this new functionallity can't access the page via the navigation. Using ForemanContext, in this PR we are updating the menu item based on the changed setting.

Ron-Lavi · 2024-01-16T18:29:52Z

Let's see if that test passes, before the last rebase it was green.
also, using one commit now :)

jeremylenz

All green now. thanks @Ron-Lavi !

ACK 👍

pr-processor bot added Not yet reviewed Waiting on contributor labels Dec 28, 2023

Ron-Lavi requested review from jeremylenz and MariaAga December 28, 2023 12:38

Ron-Lavi force-pushed the 37013-new-host-overview-menu-loaded branch from 0d3409c to 1abe167 Compare January 9, 2024 19:58

pr-processor bot removed the Waiting on contributor label Jan 9, 2024

github-actions bot added the UI label Jan 9, 2024

Ron-Lavi changed the title ~~Fixes #37013 - refresh the page after new_host_page setting is updated~~ Fixes #37013 - change the 'All hosts' menu item's url based on the new_host_page setting Jan 9, 2024

Ron-Lavi force-pushed the 37013-new-host-overview-menu-loaded branch from 1abe167 to 3999449 Compare January 10, 2024 08:40

jeremylenz reviewed Jan 10, 2024

View reviewed changes

Ron-Lavi force-pushed the 37013-new-host-overview-menu-loaded branch from 3999449 to 6fb869f Compare January 14, 2024 14:11

MariaAga reviewed Jan 16, 2024

View reviewed changes

webpack/assets/javascripts/react_app/Root/ReactApp.js Outdated Show resolved Hide resolved

Ron-Lavi force-pushed the 37013-new-host-overview-menu-loaded branch from 6fb869f to 52e071f Compare January 16, 2024 10:45

Ron-Lavi force-pushed the 37013-new-host-overview-menu-loaded branch from 52e071f to c2e5596 Compare January 16, 2024 18:24

jeremylenz approved these changes Jan 16, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Jan 16, 2024

jeremylenz merged commit 0b40a64 into theforeman:develop Jan 16, 2024
23 checks passed

tpapaioa mentioned this pull request May 8, 2024

Filter test collection based on files changed in upstream PRs. SatelliteQE/robottelo#14931

Closed

tpapaioa mentioned this pull request Jul 16, 2024

Filter test collection based on files changed in upstream PRs SatelliteQE/robottelo#15674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37013 - change the 'All hosts' menu item's url based on the `new_host_page` setting #9969

Fixes #37013 - change the 'All hosts' menu item's url based on the `new_host_page` setting #9969

Ron-Lavi commented Dec 28, 2023 •

edited

Loading

MariaAga commented Jan 2, 2024

Ron-Lavi commented Jan 3, 2024

ekohl commented Jan 3, 2024

jeremylenz commented Jan 3, 2024

Ron-Lavi commented Jan 9, 2024

ekohl commented Jan 9, 2024

Ron-Lavi commented Jan 9, 2024

Ron-Lavi commented Jan 10, 2024

jeremylenz left a comment

Ron-Lavi commented Jan 15, 2024

MariaAga commented Jan 15, 2024

Ron-Lavi commented Jan 16, 2024 •

edited

Loading

MariaAga commented Jan 16, 2024

MariaAga commented Jan 16, 2024

ianballou commented Jan 16, 2024 •

edited

Loading

jeremylenz commented Jan 16, 2024

Ron-Lavi commented Jan 16, 2024

jeremylenz left a comment

Fixes #37013 - change the 'All hosts' menu item's url based on the new_host_page setting #9969

Fixes #37013 - change the 'All hosts' menu item's url based on the new_host_page setting #9969

Conversation

Ron-Lavi commented Dec 28, 2023 • edited Loading

MariaAga commented Jan 2, 2024

Ron-Lavi commented Jan 3, 2024

ekohl commented Jan 3, 2024

jeremylenz commented Jan 3, 2024

Ron-Lavi commented Jan 9, 2024

ekohl commented Jan 9, 2024

Ron-Lavi commented Jan 9, 2024

Ron-Lavi commented Jan 10, 2024

jeremylenz left a comment

Choose a reason for hiding this comment

Ron-Lavi commented Jan 15, 2024

MariaAga commented Jan 15, 2024

Ron-Lavi commented Jan 16, 2024 • edited Loading

MariaAga commented Jan 16, 2024

MariaAga commented Jan 16, 2024

ianballou commented Jan 16, 2024 • edited Loading

jeremylenz commented Jan 16, 2024

Ron-Lavi commented Jan 16, 2024

jeremylenz left a comment

Choose a reason for hiding this comment

Fixes #37013 - change the 'All hosts' menu item's url based on the `new_host_page` setting #9969

Fixes #37013 - change the 'All hosts' menu item's url based on the `new_host_page` setting #9969

Ron-Lavi commented Dec 28, 2023 •

edited

Loading

Ron-Lavi commented Jan 16, 2024 •

edited

Loading

ianballou commented Jan 16, 2024 •

edited

Loading