Respect tour status (disabled | enabled)

Rerolled patch for 9.1.x!

Fixing failed tests!

Patch #13, 9.1 is working fine
RTBC

Rerolled #13

Putting back to RTBC seems to be a random test failure. Reran and no issues.

@clemens.tolboom under the patch you can click Add test/ retrest

The issue summary should have a proposed resolution so reviewers and committers can check that the desired solution is implemented.

Some manual testing of this would be a good idea, as well.

I do not see a code review of any of the patches. I took a brief look.

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +   * Test tour tip enable disable functionality.
   

   Tests enabling and disabling the tour tip functionality.

2. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +  public function testEnableDisable() {
   

   Ha, the test actually tests disabled first! But if this is testing the status value then use that name, testStatus.

3. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +    $tour->setStatus(FALSE);
   

   If it were me I would test enabled first, then disabled. That way we have some assurance that the disabling really removes the tips.

4. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +    // Navigate to tour-test-1 and verify the tour_test_1 tip is found with appropriate classes.
   

   Wrap to 80 characters.

5. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -29,8 +29,10 @@ abstract class TourTestBase extends BrowserTestBase {
   +   *   A boolean value.
   

   This can be more informative. We already know it is a bool from the @param above.

6. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,7 +47,10 @@ public function assertTourTips($tips = []) {
   +      $this->assertTrue(empty($tips), 'Expected no tour (disabled) on the current page.');
   

   Message are not used in core assertions, except when needed as when in a loop.

Thanks for your suggestions @quietone
Addressed the comment #26 point-1,2,4,5
Attached patch against Drupal 10.1.x
Attached reroll patch

Took a shot at updating the IS
Addressed the remaining issues from #26

Tested using https://www.drupal.org/files/issues/2018-10-07/tour_ui-2073321-3.patch
Even though the issue for tour_ui still needs work enable and disable a tour through a ui.
Once disabled I confirmed the tour was no longer appearing.
Re-enabled and confirmed tour reappeared.

This is bad behavior on my part. Moving to RTBC as this is blocking a feature or two for tour UI. And even though tour UI is not part of core there is talk that tour is going to be slated for removal. Would be nice to get this feature in before that freeze happens.

Yes, we do not self RTBC in the Drupal core issue queue. I am setting this back to needs review per Reviewed & tested by the community ["RTBC"] .

If this is block a contrib project, please add the correct tag.

For those interested, the proposal for discussing the removal of tour is at #3325445: [Policy] Remove tour module from core.

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +    // Navigate and verify the tour_test_1 tip is found with appropriate classes.
   

   nit: >80

2. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,33 @@ public function testTourFunctionality() {
   +    $this->assertTourTips([], TRUE);
   

   we can use a named argument here

3. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,7 +47,10 @@ public function assertTourTips($tips = []) {
   +      $this->assertTrue(empty($tips));
   

   can return here and avoid the elseif

Couple of minor things

Attempting to fix the issues mentioned in #33

#34 didn't address 2 or 3.

For 2 it was testing an empty but now it's passing in values.
For 3 Adding a return that way doesn't do anything as it's the end of the if statement.

Think (could be wrong) this was more what @larowlan was saying.

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +    // Navigate and verify the tour_test_1 tip is found with
   

   should this be 'is not found'?

2. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,7 +47,10 @@ public function assertTourTips($tips = []) {
   +    elseif (empty($tips)) {
   

   this can stay as an else, as an if, we returned in the above if path

Looks like #35.2 isn't handled yet, see https://php.watch/versions/8.0/named-parameters#named-param-optional-params for an example, I don't think we need to pass the empty array for the first argument as that's the default

Addressed points #37 and #35.2

1. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +
   

   This line can be removed (can be fixed on commit)

2. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +    $this->assertTourTips();
   

   Should we be passing the $tour here - to assert the one we expect to see is present?

3. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -234,6 +234,34 @@ public function testTourFunctionality() {
   +    $this->assertTourTips(expectEmpty: TRUE);
   

   👌 noice

4. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -45,8 +47,8 @@ public function assertTourTips($tips = []) {
   -    if (empty($tips)) {
   -      $this->fail('Could not find tour tips on the current page.');
   +    if ($expectEmpty) {
   +      return;
   

   I think we want

   $tip_count = count($tips);
   if ($tip_count === 0 && $expectEmpty) {
     // No tips found as expected.
     return;
   }
   if ($tip_count > 0 && $expectEmpty) {
     $this->fail(sprintf('No tips were expected but %d were found', $tip_count));
   }
   $this->assertGreaterThan(0, $tip_count);
   
   

   So we cover the three cases:

   * Expecting tips, but none
   * Not expecting tips, but found some
   * Not expecting tips, none found as expected

#39
1. Fixed
2. We can't as the $tour isn't an array
3. Thanks!
4. Updated

Test results: PASS

Notes:
- Tested the latest patch on Drupal 10.1.x using DrupalPod to setup up the env
- Tested with the umami_demo profile
- I had to uninstall and reinstall the Tour module for a tour itself to be disabled

Outcome:
1. Spun up the test environment with DrupalPod and logged in as admin
2. Verified that the Tour module was enabled in the testing site
3. Visually confirmed that tours were enabled and working by going to Languages in configuration (see Screenshot 1)
4. Changed status of tour to "false" in a yml file under language module and cleared cache
5. Checked the language tour in testing site but the tour was still visible
6. Uninstalled and then reinstalled the Tour module in the DrupalPod cli
7. Confirmed that only the language tour was disabled in the testing site (see Screenshot 2)
8. Re-enabled the language tour by setting status to “true” but it was only visible again in the testing site after I reinstalled the tour module

If you've tested and reviewed the ticket and think it's good to go feel free to change status to RTBC.

Taking #41 as a review.

This has been a block for tour_ui ability to setup tour statuses for a long time.

Committed and pushed to 10.1.x
I think this is borderline feature, so splitting the difference and marking as a task.

Didn't backport because we're in RC phase and I don't think this meets the requirements for commit during RC