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

[UX] Running cron via the Admin Bar menu always redirects to the "Status report" page #3973

Open
klonos opened this issue Aug 9, 2019 · 11 comments

Comments

@klonos
Copy link
Member

klonos commented Aug 9, 2019

Steps To Reproduce
Run cron via the "Run cron" menu item, under "Home"...

Actual behavior
You get redirected to /admin/reports/status and get a "Cron ran successfully." message

Expected behavior
You should stay on the same page.


PR by @BWPanda - backdrop/backdrop#2900

@findlabnet
Copy link

This is bug? Not sure. After run cron you can see if something going wrong on status report page.
I run cron manually for this purpose only.

@indigoxela
Copy link
Member

I agree with @findlabnet - this isn't really a bug.

Sometimes it could be better to not get redirected, though. If - for instance - something's weird on a page (a view) and the admin want's to check if a cron run fixes it.

On the other hand the status report page could offer helpful information.

Personally, I don't have any preference regarding a redirect. Both is OK for me.

@klonos
Copy link
Member Author

klonos commented Aug 12, 2019

...for instance - something's weird on a page (a view) and the admin want's to check if a cron run fixes it.

That's actually the experience I've had twice before I filed this as a bug.

For the record, the D7 version of the admin_menu module only "redirects" to the "Status report" page if the core Overlay module is enabled (but we have removed the Overlay module from Backdrop). Even then, that is not redirecting exactly, as what happens is that the "Status report" page loads in an overlay/popup, which the user can easily close, to get back to the page they were when they run cron from:

Screen Shot 2019-08-12 at 4 55 56 pm

...if the user runs cron (either via the admin_menu menu, or via the "No update info available" warning) while the Overlay module is disabled, they remain in the page they initiated the cron run from:

Screen Shot 2019-08-12 at 4 49 27 pm

Screen Shot 2019-08-12 at 4 49 43 pm

So I still think this is a bug, and would expect this to work the same way that clearing caches via the Admin Bar menu works. Lets wait for more feedback before we action this though.

@indigoxela
Copy link
Member

the D7 version of the admin_menu module only "redirects" to the "Status report" page if the core Overlay module is enabled

Are you sure? I always disable the overlay module in D7. Running cron via the admin_menu always redirects me to the status page.
Clearing caches doesn't.

In this case Backdrop behaves exactly the same as D7 with admin_menu.

@klonos
Copy link
Member Author

klonos commented Aug 13, 2019

Are you sure?

You are right. I have tested this in simplytest.me, with admin_menu 7.x-3.0-rc6.

Here's what I've tried once the sandbox was spun up:

  1. Head to /admin/modules -> disable the core Toolbar module -> enable the Admin Menu Toolbar Style module
  2. Head to home page
  3. clear cache via the Admin Menu -> no page redirect
  4. run cron via the Admin Menu -> overlay/popup with the "Site status" page, and a Cron run successfully success message; no page redirect.
  5. Close overly/popup -> still in the home page
  6. Head to admin/modules -> disable the core Overlay module
  7. Clear caches -> success message / no redirect
  8. Run cron via the link in the "No update information available" warning that also shows up as a result of clearing caches -> success / no redirect
  9. Run cron via the Admin Menu link -> success / redirect to status page.

@ghost
Copy link

ghost commented Sep 25, 2019

I think people shouldn't be redirected after running cron. If the only reason is to see if there are issues after doing so, then that's what the notification badge is for.

Let's remove the redirection and make everything consistent (since clearing caches and running cron via other methods don't cause a redirect).

@ghost
Copy link

ghost commented Sep 25, 2019

Here's a simple PR.

@klonos
Copy link
Member Author

klonos commented Sep 25, 2019

If the only reason is to see if there are issues after doing so, then that's what the notification badge is for.

👍 to that!

...the PR is very simple indeed, and works fine, but I would like us to consider what happens when cron fails to run (unfortunately, I don't know how to simulate that 🤔).

Also, there is still a backdrop_goto('admin/reports/status'); in system_run_cron(). So in theory, if you run cron via any other way, the redirection still happens(??). I have tested running it via /admin/config/system/cron and there is no redirection though 🤷‍♂

@klonos
Copy link
Member Author

klonos commented Sep 25, 2019

...perhaps when cron fails, it makes sense to be redirecting to /admin/config/system/cron? Not sure.

@ghost
Copy link

ghost commented Sep 26, 2019

So there's three places that run cron:

  • Admin bar (via system_run_cron())
  • Cron page (/admin/config/system/cron) (via system_run_cron_submit())
  • Status page (via system_run_cron())

system_run_cron_submit() just redirects to itself and system_run_cron() redirects to the Status page (without any redirection you're left on a blank menu callback page).

I've updated my PR to simplify this so that system_run_cron_submit() just calls system_run_cron(), and system_run_cron() now redirects back to the original page if there's a referrer, or to the Status page otherwise.

@ghost
Copy link

ghost commented Sep 26, 2019

Also, if this PR doesn't get in (or my change to system_run_cron_submit() is reverted), then the wording in https://github.com/backdrop/backdrop/blob/1.x/core/modules/system/system.admin.inc#L1593 needs to be fixed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bugs
  
Big bug bucket
Development

Successfully merging a pull request may close this issue.

3 participants