Individual blog posts are not commonly added to menu

Hi,
I have disabled the main navigation from the Blog content type and raised an MR.

Please review.
Thanks,

Looks good except for one thing.

Thanks for the review @phenaproxima.
I have made the suggested changes.

Kindly review.
Thanks,

That looks good to me!

Wait a sec, I need to look at this more closely. This MR is adding menu_ui settings where none exist...does that mean, if there are no menu_ui settings, it defaults to putting everything in the main menu??

I need to get an answer there. If that is the case, we'll probably want some test coverage too, and we might want to apply this change to more than just the Blog content type.

OK, I found the spot where menu_ui does this: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/menu_...

So if you don't specifically specify "no menus" it assumes you want the main menu as an option. That's kind of a dopey assumption, IMHO, but it means this change makes sense.

I think it also means we should rescope it so that this applies to more content types, and is covered by tests. I would imagine that most content types, except for Page, wouldn't normally be in the menu. So we should probably add this same change to every content type, and add the same kind of test coverage (log in as a content_editor, visit the /node/add form for that content type, confirm that there are no menu options) to each content type recipe's ComponentValidationTest.

I think this could use product-level input on which content types should have the ability to be added directly to the menu, and which shouldn't. Assigning to @pameeela for that.

In our product strategy at 1x, we absolutely do not allow all content types to be added to the menu. Only the basic page. Content types like news, events, person etc can not be added. Therefore I agree with @phenaproxima and confirm that this is very well accepted by all users of our 1x Drupal CMS.

Yes I agree only basic page should have menu enabled by default, so the same change should be made for the other content types.

Great! Thanks @pameeela. That seems very clear - let's disable the menu for every content type except Page, and add test coverage to confirm that we've done what is needed.

Hi,
I have updated the test coverage as per the requirement.
Kindly review.

That's a good start! But, I would like the tests to be approached a little differently. Right now they're testing the menu_ui config directly, which isn't very useful and doesn't make a ton of sense. After all, we know what the config will be; we can read the YAML files for ourselves. What we want to test is: will the config have the desired effect? Will users have the experience we want them to have?

In other words, what the tests should do:

• Log in as a content editor
• Visit the /node/add/WHATEVER page
• Confirm that the menu settings don't show up

Hi @phenapromixa, I have modified the test to match your suggestions.
I see the pipeline is still not green, but I cannot see any errors in my local. Can you please check and let me know what is the issue here?

The failures are probably random, due to an ongoing problem where MySQL fails to start. You can tell because, if you look at the job logs, you'll see things like "InstallerException...resolve the problems before continuing", and it will mention MySQL. It's not your fault. I usually just rerun the failing ones until they pass. :)

As for the tests, this is definitely a more correct approach - we're now asserting that the user experience is what we want. However, we can do this even more simply, and robustly.

Rather than write new tests for each ComponentValidationTest, let's create a single new test, in drupal_cms_starter...call it ContentEditingTest or similar. In it, have a single test method which does what you're already doing (except don't bother asserting the presence of the content_editor role, or that the created account has it -- the test will fail anyway if either of these conditions isn't true). This new test method should use a @testWith annotation to loop through every content type and test it -- that will keep things compact, and prevent the need for code duplication :) See \Drupal\Tests\drupal_cms_content_type_base\Functional\ContentDuplicationTest::testContentDuplication() for an example of how this works.

We should also ensure that, for the Page content type, we test that we do see the menu settings. So the method will probably have a signature like this:

  /**
   * @testWith ["drupal/drupal_cms_blog", "blog", false]
   *   ["drupal/drupal_cms_case_study", "case_study", false]
   *   ["drupal/drupal_cms_events", "event", false]
   *   ["drupal/drupal_cms_news", "news", false]
   *   ["drupal/drupal_cms_page", "page", true]
   *   ["drupal/drupal_cms_person", "person", false]
   *   ["drupal/drupal_cms_project", "project", false]
   */
  public function testMenuSettings(string $recipe_name, string $content_type, bool $has_menu_settings): void {
    // the test code in here...
  }

Really coming along! Thanks for sticking with it. If we approach this way, we don't need to repeat the same test in every individual recipe.

Hi @phenaproxima,
Thanks for the help.

I have created a separate test file named ContentEditingTest under drupal_cms_starter.
And It passes the pipeline.

Please review.

This is super close to ready. Looks great - almost exactly what I was hoping for. Real strong test coverage here. Only three points of feedback!

I made the final minor changes here just to get this in - it's great work! Thanks everyone. Merged into 1.x.

Updated title because it's not true that they're not allowed, just that the option isn't in the node form.

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