Problem/Motivation

Tour is being deprecated, see the parent issue for details. This issue is for removing tour from the umami profile.

For an example see how this was done for the RDF module in #3243121: Remove RDF module from the Standard profile.

Steps to reproduce

Proposed resolution

Remove Tour module from the Umami profile.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

TBA

Issue fork drupal-3376339

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

smustgrave made their first commit to this issue’s fork.

smustgrave opened merge request !4456

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

So umami had css and a config file.

I deleted the CSS and moved the config file.
Also moved the one test function that was related to tour

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue tags: +Needs subsystem maintainer review

Wonder if this needs sign off from Umami maintainer.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

So moving the config is the issue. But not sure what to do for it.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Moving the config to optional feels appropriate to me

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

That’s what I did but the config has a dependency to demo_umami module. Should this config be deleted?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

Went ahead and deleted the config. demo_umami isn't a module that I can tell. And the workflow would have to be

Someone installs umami
Download contrib tour
Not sure tour as a contrib really should maintain config for a demo profile.

markconroy’s picture

markconroy CreditAttribution: markconroy at Annertech commented

Thanks for working on this. I've no issue with our Tour items being removed if Tour is being removed from core. But let's definitely not support Tour as a contrib module in our demo_umami profile - so let's remove _all_ of Tour from Umami and not have some of it supported via "optional" config.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs work
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

@andypost think all the tour stuff has been removed

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Instead of removal the Umami's tour could live in optional config of the module

core/profiles/demo_umami/config/install/tour.tour.umami-front.yml

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

So I tried that but I couldn't get around https://www.drupal.org/pift-ci-job/2722966

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
Related issues: +#3270445: Drupal\KernelTests\Config\DefaultConfigTest should test deprecated modules separately, not ignore them, +#2834250: Core schema tests (DefaultConfigTest and TestViewsTest) can't reliably check contrib.

Reverted commit with removal - that's just a test which needs more fixes, so I excluded the profile and added todo to existing issue #2834250: Core schema tests (DefaultConfigTest and TestViewsTest) can't reliably check contrib.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs subsystem maintainer review

Works for me

Started CR here https://www.drupal.org/node/3377475 (just copied Standard CR)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work

Added comment to the MR.

I don't think we actually need CR for this since this should not impact anyone. Umami is the demo experience and we are allowed to change that as we see fit.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review

removed test

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Moving to needs work for the test restoration.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs work » Needs review

So instead of writing a new test module just used the toolbar module. Locally this passed.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks @smustgrave, all threads answered, code changes look OK to me => RTBC

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed

Committed 12be8e7 and pushed to 11.x. Thanks!

Does not backport cleanly to 10.1.x due to PerformanceTest, so let's just leave tour in Umami in 10.1.x, it doesn't harm anything.

  • longwave committed 12be8e77 on 11.x 
    Issue #3376339 by smustgrave, andypost, quietone, markconroy, lauriii,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Published the change record.