admin/reports/upgrade redirect doesn't handle view arguments when enabled

This should do it then. Thanks for reviewing

I didn't remove the custom access check. I just replaced the one on the reports route with the permission to access site reports. Which seems consistent with the other report routes to me.

migrate_drupal_ui.upgrade:
  path: '/upgrade'
  defaults:
    _form: '\Drupal\migrate_drupal_ui\Form\MigrateUpgradeForm'
    _title: 'Upgrade'
  requirements:
    _custom_access: '\Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess'
  options:
    _admin_route: TRUE

migrate_drupal_ui.log:
  path: '/admin/reports/upgrade'
  defaults:
      _controller: '\Drupal\migrate_drupal_ui\Controller\MigrateController::showLog'
  requirements:
     _permission: 'access site reports'
  options:
    _admin_route: TRUE

Adding to patch#6 chaining permission and custom_access.

@dev.patrick, thanks for making the patch.

+++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
@@ -16,6 +16,9 @@ class MigrateController extends ControllerBase {
+	if ($this->moduleHandler()->moduleExists('views')) {

Whitespace error. If not aligned.

Thanks @Quietone, corrected white space error.

@dev.patrick, your welcome. But...

+++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
@@ -16,6 +16,10 @@ class MigrateController extends ControllerBase {
+	

the pesky whitespace moved to here!

My bad. Using windows and has some issue with IDE. Really appreciate if you give another chance.

This looks good. I tested it manually and it works. We should add automated testing, that shouldn't be too hard.

+++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
@@ -16,6 +16,9 @@ class MigrateController extends ControllerBase {
+    if ($this->moduleHandler()->moduleExists('views')) {

This is rather tricky. Even if Views module exists it does not guarantee that the Watchdog view is in place. Users are free to remove/modify any views.

Would adding another conditional for checking if the view/display is available and enabled help?

Added checks to patch from #15 to make sure that:

• Watchdog view exists
• Its Page display exists and is active

Setting to "Needs Review"

Do you have any tips on how to structure such a test? I haven't written any tests before.

Alright, new patch includes tests, fixes a couple of minor errors, and removes the redundant permission check on the route since it's handled by \Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess

Coding standards

Alright one more try! Removing whitespace

Adding test group -- I think this should do it.

Some refactor over #33 patch that solve points 1, 2, 4 about #34.

I don't understand well this point please more details:
"3. I don't see any asserts in these 2 tests that we are actually seeing filtered results. Which is sorta the intent of this issue. "

I think we are getting a bit jumbled up, here. Don't have my machine atm but here are some notes.

Here we're testing that we redirect to the view if it is enabled. But we don't have test a for if the view isn't enabled. Could we add a test of the url if the view is disabled too?

You're referencing the assertion in testRedirectDestinationWithoutView() which is the test without the view enabled -- it's being deleted prior to asserting the url. Did you mean to say that we need to add a test with the view enabled?

@vacho, why'd you remove testRedirectDestination()? It could be renamed to testRedirectDestinationWithView(), I suppose, but we need two separate tests in here.

Changes done according to #37.
@heddn Please review.

Just looking at the documentation.

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +   * Disable the Watchdog view and then tests whether or not
   +   * the user is redirected when navigating to admin/reports/upgrades
   

   Sentence doesn't span 80 columns correctly and is missing a full stop.

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +  public function testRedirectDestinationWithDisableView() {
   

   Method needs a summary line.

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +  public function testRedirectDestinationWithoutView() {
   

   Missing summary line

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +   * Click a disable link to perform a disable operation on a view.
   

   s/Click/Clicks/

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +   * @return
   

   Missing return type

6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +   *   The views page content that results from clicking on the disable link, or FALSE on
   

   Line > 80 characters.

7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,102 @@
   +   * @param $label
   ...
   +   * @param $href_part
   

   Needs a datatype

Done changes according to #42,
@quietone, Please review.

Assigning to myself for review and testing

@vsujeetkumar, thanks for continuing with this.

I think this is missing a test. There are currently two tests, one with the view disabled and one with the view deleted. There needs to be a test with the view enabled.

1. +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -16,6 +17,15 @@ class MigrateController extends ControllerBase {
   +          if (!empty($page_display['display_options']['enabled'])) {
   +            return $this->redirect('dblog.overview', ['type[]' => 'migrate_drupal_ui']);
   +          }
   

   During manual testing and running MigrateControllerTest the return statement was never reached. Perhaps my testing was not thorough but the tests certainly should be.

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,111 @@
   + * Provides a base class for testing the Migrate Controller.
   

   This is testing the Migrate Controller but it isn't a base class.

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,111 @@
   +    $this->assertResponse(200);
   ...
   +    $this->assertUrl('admin/reports/dblog?type%5B%5D=migrate_drupal_ui');
   

   assertResponse() and assertUrl() are deprecated, Lets use whatever the recommended replacements are.

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,111 @@
   +  public function clickViewsDisableLink($label, $href_part) {
   

   This method is used once which makes we wonder if it is really needed. Plus the return value is not used.

Done the changes advised in #45,

1: Added the test for enable view.
2: updated the class summery.
3: Fixed the deprecated assertResponse() and assertUrl() issues.
4: Rename the method "clickViewsDisableLink" to clickViewsOperationsLink and now it is used in multiple places, Also it is required for disable and enable the view.

@quietone Please review.

Please ignore the previous interdiff(interdiff_43-46.patch), I have added correct interdiff.

Thanks for the fixes.

This time I looked closer at comments.

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +   * Disable the watchdog view and redirect to the dblog from upgrade path.
   +   *
   +   * Disable the Watchdog view and then tests whether or not the user is
   +   * redirected when navigating to admin/reports/upgrades.
   

   The summary line and the summary are virtually identical. This is a simple test I think a summary line is sufficient. Maybe 'Tests the upgrade path with the watchdog view disabled and enabled.' The summary line should follow the coding standards, Drupal API documentation standards for functions

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +    $this->clickViewsOperationsLink($this->t('Disable'), '/watchdog/');
   

   clickViewsOperationsLink returns a value, why is that not used?

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +    $this->assertSession()->addressEquals('admin/reports/dblog?type%5B%5D=migrate_drupal_ui');
   

   Can the address be a constant?

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +    $this->clickViewsOperationsLink($this->t('Enable'), '/watchdog/');
   

   The last page navigated to was 'admin/reports/upgrade' which does not have an 'Enable' button. Am I missing something?

5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +    // Tests whether or not the user is redirected with the enable view when.
   +    // Navigating to admin/reports/upgrades.
   

   It looks like this is supposed to be one sentence, not two. It can be simpler, 'Tests redirection to report page when the watchdog view is enabled.'

6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +   * Delete the watchdog view and redirect to the dblog from upgrade path.
   +   *
   +   * Deletes the Watchdog view and then tests whether or not the user is
   +   * redirected when navigating to admin/reports/upgrades.
   

   Same here the summary line and summary are virtually the same.

7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +    $this->assertSession()->statusCodeEquals(200);
   

   Is this a sufficient test? Wouldn't it be better to test for ' The view Watchdog has been deleted.'

8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +    // Tests whether or not the user is redirected when.
   +    // Navigating to admin/reports/upgrades.
   

   Should be one sentence and wrapped correctly.

9. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,119 @@
   +   * Click a view link to perform an operation.
   

   s/Click/Clicks/

10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
    @@ -0,0 +1,119 @@
    +   *   Text between the anchor tags of the disable link.
    

    The summary says that this method will 'perform an operation' yet here it specifically says it is a disable link. Which is correct?
    I've reread your comment and I see you changed the name of this method. Why is a name change needed here?

11. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
    @@ -0,0 +1,119 @@
    +   * @return
    

    Needs a return type.

12. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
    @@ -0,0 +1,119 @@
    +   *   The views page content that results from clicking on the link or,
    +   *   FALSE onfailure.
    

    Not wrapped correctly and change 'onFailure' to 'on failure'. Would be good to add what causes a failure. I don't see that this returns page content, either. Oh, I see, it used to return a boolean TRUE or FALSE. What is the reason for changing that?

Here is a patch for the points in #48.

Oops, forgot to uncomment a line in the test.

Random test failure, Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest. Setting back to NR.

The failure occurs since #3164686: WebAssert::addressEquals() and AssertLegacyTrait::assertUrl() fail to check the querystring was committed. I have updated the test to reflect that. Tested manually as well.

+++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
@@ -16,6 +17,15 @@ class MigrateController extends ControllerBase {
+    if ($this->moduleHandler()->moduleExists('views')) {
+      if ($view = Views::getView('watchdog')) {
+        if ($page_display = $view->storage->getDisplay('page')) {
+          if (!empty($page_display['display_options']['enabled'])) {

I'm thinking that this can be replaced with

    if ($this->config('views.view.watchdog')->get('status')) {
      return $this->redirect('dblog.overview', ['type[]' => 'migrate_drupal_ui']);
    }

If views is not installed, $this->config('views.view.watchdog')->get('status') returns NULL. This seems a much simpler way to determine if the watchdog view is available.

Thank you @quietone for the patch. I have updated the #56 patch as suggested by @heddn.

1. +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -16,6 +16,9 @@ class MigrateController extends ControllerBase {
   +    if ($this->config('views.view.watchdog')->get('status')) {
   +      return $this->redirect('dblog.overview', ['type[]' => 'migrate_drupal_ui']);
   +    }
        $_SESSION['dblog_overview_filter'] = [];
        $_SESSION['dblog_overview_filter']['type'] = ['migrate_drupal_ui' => 'migrate_drupal_ui'];
        return $this->redirect('dblog.overview');
   

   I don't think hardcoding config IDs here is correct. I don't think we should do the if at all. We can do something like:

       // Set both session and a query parameter so both with the watchdog view and
       // the fallback.
       $_SESSION['dblog_overview_filter'] = [];
       $_SESSION['dblog_overview_filter']['type'] = ['migrate_drupal_ui' => 'migrate_drupal_ui'];
       return $this->redirect('dblog.overview', [], ['query' => ['type' => ['migrate_drupal_ui']]]);
   

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,104 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp(): void {
   +    parent::setUp();
   +
   +    // Log in as user 1. Migrations in the UI can only be performed as user 1.
   +    $this->drupalLogin($this->rootUser);
   +  }
   

   I think we should be creating a message for testing purposes here so we can test that the type is selected - something like:

       // Create a migrate message for testing purposes.
       \Drupal::logger('migrate_drupal_ui')->notice('A test message');
   

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,104 @@
   +  /**
   +   * Tests the upgrade path with the watchdog view disabled and enabled.
   +   */
   +  public function testRedirectDestination() {
   +    // Disable the watchdog view.
   +    $this->drupalGet('admin/structure/views');
   +    $this->assertTrue($this->clickViewsOperationsLink('Disable', '/watchdog/'));
   +    $session = $this->assertSession();
   +    $session->statusCodeEquals(200);
   +
   +    // Tests redirection to report page when the watchdog view is disabled.
   +    $this->drupalGet('admin/reports/upgrade');
   +    $session->addressEquals('admin/reports/dblog');
   +
   +    // Enable the watchdog view.
   +    $this->drupalGet('admin/structure/views');
   +    $this->assertTrue($this->clickViewsOperationsLink('Enable', '/watchdog/'));
   +    $session->statusCodeEquals(200);
   +
   +    // Tests redirection to report page when the watchdog view is enabled.
   +    $this->drupalGet('admin/reports/upgrade');
   +    $session->addressEquals('admin/reports/dblog?type%5B%5D=migrate_drupal_ui');
   +  }
   +
   +  /**
   +   * Tests the upgrade path with the watchdog view deleted.
   +   */
   +  public function testRedirectDestinationWithoutView() {
   +    // Delete the watchdog view.
   +    $this->drupalGet('admin/structure/views/view/watchdog/delete');
   +    $session = $this->assertSession();
   +    $session->statusCodeEquals(200);
   +
   +    $this->submitForm([], 'Delete');
   +    $session->statusCodeEquals(200);
   +    $session->pageTextContains('The view Watchdog has been deleted.');
   +
   +    // Tests whether or not the user is redirected with the view is deleted.
   +    $this->drupalGet('admin/reports/upgrade');
   +    $session->addressEquals('admin/reports/dblog');
   +  }
   

   I don't think we should have 2 tests here. It's not necessary. We should also be testing the whether or not the correct type is selected rather than the URL. I.e. we should be testing what the user wants to see - they don';t care whether it is session or a query parameter is what makes it work.

   So something like:

     /**
      * Tests the upgrade reports with the view enabled, disabled and uninstalled.
      */
     public function testUpgradeReport() {
       $session = $this->assertSession();
   
       $this->assertTrue(View::load('watchdog')->status(), 'Watchdog view is enabled');
       // Tests redirection to report page when the watchdog view is enabled.
       $this->drupalGet('admin/reports/upgrade');
       $session->optionExists('type[]', 'migrate_drupal_ui')->isSelected();
       $session->pageTextContainsOnce('A test message');
   
       // Disable the watchdog view.
       $this->drupalGet('admin/structure/views');
       $this->assertTrue($this->clickViewsOperationsLink('Disable', '/watchdog/'));
       $session->statusCodeEquals(200);
   
       // Tests redirection to report page when the watchdog view is disabled.
       $this->drupalGet('admin/reports/upgrade');
       $session->optionExists('type[]', 'migrate_drupal_ui')->isSelected();
       $session->pageTextContainsOnce('A test message');
   
       \Drupal::service('module_installer')->uninstall(['views_ui', 'views']);
       // Tests redirection to report page when views is uninstalled.
       $this->drupalGet('admin/reports/upgrade');
       $session->optionExists('type[]', 'migrate_drupal_ui')->isSelected();
       $session->pageTextContainsOnce('A test message');
     }
   

Working on it.

I have updated the #58 patch as suggested by @alexpott.

I reviewed the patch and the changes suggested in #60 have been made. There are no coding standard errors. So, I though this was ready but then I noticed two problems with the comments.

1. +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -16,9 +16,11 @@ class MigrateController extends ControllerBase {
   +    // Set both session and a query parameter so both with the watchdog view and
   +    // the fallback.
   

   This sentence doesn't make sense. Maybe something like 'Set both the session and query parameter so both the watchdog view and the fallback work correctly.' I am not sure

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerTest.php
   @@ -0,0 +1,96 @@
   +   * Tests the upgrade reports with the view enabled, disabled and uninstalled.
   

   I think reports should be singular here, or am I wrong? s/reports/report/.

Made changes as per the above suggestions. please review.

Committed and pushed 0a8bf19ac6 to 9.1.x and 4190ccb3d8 to 9.0.x. Thanks!

I think this worth backporting to 8.9.x as it helps people with migrations.

Removing the void.

Committed 606eae5 and pushed to 8.9.x. Thanks!