Convert book.module hook_help() to topic(s)

Parent issue was committed, so we can un-postpone this now!

Attached is a patch the adds help_topics for core's Book module.

To verify:
Apply the patch
Enable the Book module
As a logged in administrator, visit /admin/help of your site
Under the Topics section verify Books is listed.

Click on the link, you will be taken to /admin/help/topic/book.about

Verify "What are Books?" is displayed, and the list of related topics are there.

Tagging this for DrupalCamp Asheville. We are having a mentored contributor workshop and review of this issue would be a perfect novice task.

Thanks!

Can I be as bold to ask that this issue be reserved for the workshop??

Since someone already made a patch... you could review and/or revise the patch, but there is someone already working on it... maybe a few of the others that don't already have patches?

I'm verifying the verification steps from #5 in order to build up a 'script' for first-time contributors in Asheville and found out there's a missing step to enable Help Topics experimental module before checking admin/help path.

As a script, I'd suggest the following these steps review the child issues:
[target] := the module that is being converted to a topic.
[target-main] := the starting page of the converted topic.
[target-main-title] := the main title of the converted topic page.
1. Apply the patch (or use simpletest.me).
2. Enable the [target] module and make sure 'Help Topics' experimental module is enabled as well.
3. As a logged-in administrator, visit /admin/help of your site.
4. Under the 'Topics' section verify [target] is listed.
5. Take a screenshot that verifies the new [target] is present on this page.
6. Click on the link of the newly converted core module, you will be taken to /admin/help/topic/[target-main]
7. Take a screenshot that verifies the main page of [target].
8. Verify "[target-main-title]" is displayed, and the list of related topics are there too.

Good point, thanks! We will need to update the template issue #3041928: ISSUE TEMPLATE -- Do not edit -- template for making child issues and the other issues that have already been created. ... hm...

But I'm not sure about these exact steps. In some cases, we may not be making "top-level" topics, in which case they will not be listed on the admin/help page directly. Which is OK. Our policy is you should either create a top-level topic, or if it is more appropriate, create a non-top-level topic and mark it as "related" to an existing top-level topic.

And we definitely don't want to end up with a 1-to-1 copy of the module overviews as "topics". Some modules do not need a whole topic to themselves; others may need several. We want to have task-centered topics. So that needs to be part of the review. Just taking a screenshot is not enough. We really need to review the content of the topic(s) and make sure it is actually good, task-centered documentation. The titles should also reflect this -- we don't want a topic called "The xyz module" at all. That is not a task. So... these steps need some reconsideration.

On admin/help under "Topics" should "Books" be replaced by a more descriptive title for the tasks? Something like "Documenting your site with Books" so new users quickly understand what they can achieve with that topic.

Some notes from a session when the steps got followed:
step2: enabling a module is not necessarily straight forward to a non-dev contributor, so the suggestion is: "Enable the [target] module by visiting admin/modules page or using drush."
step7 and 8 might be valuable to be merged as those should be executed together.
+1 step for the content review might be written here too summarizing in a short way what we want to achieve as explained in #10.

@luwoldy, @eeyorr, @ tatarbj, @ volkswagenchick we've all collaborated about the one to one comment. We feel that the text does need to be modified, but unsure how the text should read. We did test the links and all work. The patch applied successfully using the simplytest.me
Will come back to this tomorrow when more contributors are available.

@luwoldy, @eeyorr, @ tatarbj, @CelSki, and I did discuss the copy of the patch at length. We reviewed the steps for reviewing the patch in the UI.

1. Apply the patch via git (or use simpletest.me).
2. As a logged-in administrator navigate to Administration > Extend (admin/modules). Enable the 'Help Topics' experimental module and [target] module.
3. Navigate to Administration > Help (/admin/help).
4. Under the 'Topics' section verify [target] is listed.
5. Take a screenshot that verifies the new [target] is present on the page.
6. Select the link of the newly converted core module, the user should be directed to /admin/help/topic/[target-main]
7. Take a screenshot that verifies the main page of [target]. Please review for grammar, spelling, and usability. Review links and ensure they are correct and relevant.
8. Verify "[target-main-title]" is displayed, and the list of related topics are there too.

Tagging issue for DrupalCamp Colorado Contribution Day August 4, 2019 -https://2019.drupalcampcolorado.org/contribution-day

The steps in #14 for reviewing patches have some problems:

a) step 4: [target] may not be a "top-level" topic. Only top-level topics are listed on admin/help. Topics that are not top-level are instead found by navigating via another topic that they are listed on as "Related". So, I think it's important to explain how to visit topic by its machine name, rather than taking that information out of the review steps and saying to always get there by a link on admin/help.

b) It looks like [target] is being used as both the name of the module (step 3) and the name of the help topic (step 4). There are two problems with this: (a) there is often (and should not be) a one-to-one relationship between modules and topics and (b) the module name is not a good title for the topic in any case. To expand on (a), some patches may be adding information to existing topics rather than writing new topics, and some other patches may be making multiple Twig files.

c) The text review (step 7) is incomplete. See issue summary review steps 4 and 5 for the other things that need reviewing. So it should be: grammar, punctuation, spelling, all in-text links work. Plus a text review to verify it's task-oriented help, concepts/explanations are separate from tasks in sections with "what is/are" headers, the task steps work if you follow them, and all the information that is useful from the old hook_help is present in the new topics.

d) Do we really need the review to include making screenshots of admin/help and of the help topic page? I worry that encouraging novice contributors to make screenshots makes them think that just making a screenshot is an acceptable review -- I've seen a lot of reviews of patches that included a screenshot and no information about whether the person reviewing actually tested the functionality of the patch. Sometimes people even just make a screenshot of their command line showing that the patch applied, and don't add other information. I think it's really important when making a review to state clearly what you tested as well. Or in this case, what you verified (that the link to the topic is either directly on admin/help or on the Related topics), and what textual reviews you did of the topic.

I worked off @thejimbirch's patch and used Acquias video on the module to come up with the content. First time patching so please let me know if I fudged anything up.

Hi, thanks for the patch -- it's a great start! It has some problems though:

a) The Book module is not being used to produce the User Guide, so that link to "see it in action" is incorrect. Actually, that whole top-level topic seems like kind of a marketing pitch to me, rather than straightforward documentation... Everything in our topics should either be categorized as "concept" documentation (explaining what something is, with a heading like "What is a book?" and text that describes things rather than markets them or explains how to do thing), or "task" documentation (explaining how to do something, with a heading like "Creating a book", with numbered steps).

b) We should not be explaining how to place blocks in the Book module topics. That belongs in the Block module topics.

c) The topics that are inside core/modules/book/help_topics will not be visible unless the Book module is already installed. So, we should not have a topic that tells you to enable the Book module. The rest of that topic on creating books looks pretty good though!

d) I am not sure the topic on Collaborating is needed... It seems like maybe instead a topic about "Managing book permissions" would be more useful? It could describe what the various permissions are and what they allow people to do.

e) We haven't really set too many standards for help topics... but I think it would be useful to adopt the text formatting and navigation description standards we have for the User Guide. Such as:
- Any text you see in the Drupal UI should be in italics (EM tag)
- Navigation should be described using > and starting at the top, such as "In the Manage administration menu, navigate to Structure > Content types".

So... Maybe the content you've written just needs to be trimmed down and reorganized a bit. We could have topics as follows:

1. Top-level topic called "Creating books of documentation" or something like that. It could have an H2-level heading called "What is a book?" that would explain the concept of books.

2. Non-top-level topic called "Managing book permissions". This might include a DL list that lists the permissions as DT elements and describes each one in DD elements.

3. Non-top-level topic called "Creating and editing books" that would include H2-level headings like "Creating a new book" and "Adding new content to a book".

4. Non-top-level topic called "Displaying and printing books" that would include H2-level headings like "Printing a book" and "Displaying the table of contents for a book" [that one would tell you to place the "Book navigation" block, and link to the Block topic (which doesn't yet exist probably) for more information about placing blocks).

Again, thanks! I really think this was a great start -- it just needs a little reorganization and trimming.

Updated issue summary with better instructions/guidelines

I'll provide a new version that addresses the point made in #18

You might want to take a look at #3041928-15: ISSUE TEMPLATE -- Do not edit -- template for making child issues starting at comment #15. We're probably going to add a bit to this issue summary, to define 2 types of topics. Some will be "task" topics, and others will be "section" topics (like Chapters in a book). Proposed standard is there, but it hasn't been adopted yet. Sorry for the moving target! Hopefully the issue summaries on these issues is getting clearer, and the standards are getting clearer...

Here are the new guidelines/instructions!

Here is a new patch addressing the feedback from #18 - #22.

I tried to use as much as possible to text from earlier patches, but it was difficult to keep it intact, given the various requirements. I am afraid the result is a patch that contains many changes compared with the previous one.

Regarding the feedback in #19:
a) the marketing talk is gone
b) instructions on configuring blocks have been kept in the book.configuring topic
c) notes on enabling the module have been removed
d) a complete list of permissions has been in the book.configuring topic
e) I tried to stick to standards. Let's see if we come up with some kind of a style guide for referring to Drupal UI

Regarding the structure I ended up with a Concept topic and 2 action topics (configuring and editing books).

Regarding the moving target: that is OK, I guess there is still plenty of work to be done. I feel the book module is good candidate to figure out what we really want with the Help Topics.

I formatted the step by step instruction as a bullet list.

We've migrated the help topic standards to https://www.drupal.org/docs/develop/documenting-your-project/help-topic-... so updating issue summary again.

- Patch applied. Help Topics module and Book module were enabled.
- I found few typos there and marked them in screenshots.
- All links work!
- I see, that you followed suggestions in comment #18, so I'll wait on @jhodgdon structure review.
Thanks!

Thanks for the patch and review! A few notes from my end:
- I definitely like the About topic! Nice clear definitions, and all the terminology included.
- The task topics are not following the heading structure that is in our current help topic standards on https://www.drupal.org/docs/develop/documenting-your-project/help-topic-...
- It looks like there are a lot of different tasks described each of the task topics... Each task should be in its own topic instead of combining them together, according to our standards
- I think the steps should be OL lists rather than UL lists

I have split the task into different topics.

This is getting better, but please check out the help topic standards: https://www.drupal.org/docs/develop/documenting-your-project/help-topic-...

Standards problems I found in this patch (I haven't reviewed the text in detail yet):

a) Each Task topic should have:
- 1 goal
- 1 set of steps

So for example the topic "Configuring books" in this patch seems to describe 4 different tasks. It needs to be split up.

b) The headings in a task topic are given in the standards document too. These are not being followed, for example in the topic "Creating books", there are headings called "Creating books" (shouldn't be there), and no "Goal" or "Steps" that should be there.

c) Navigation is not being described correctly.

That's what I found from a quick look... this needs more work. Thanks!

Tagging for badcamp2019, thanks! (October 2-5)

We just found out that all topic Twig files currently need to go into core/modules/help_topics/help_topics (with their finalized module-based file names), for the time being until the Help Topics module is stable. Updating issue summary. Patch will need to be updated too.

#30 i created patch as per new help topic standers please review.

Please ignore #34 as there was some syntax error. i re-created patch please review.

It looks like the automated syntax test are failing because topic book.permissions says it is related to a topic book.organizing_book, which does not exist.

Hii

I have created the patch by correcting the syntax errors and some grammar, punctuation errors
Please review.

Unassigning @alonaoneill from the issue, as we prepare this issue for first time contributor workshop and alonaoneill did not work on the issue since 3 month, and others have worked on it. Having an old and inactive assignment might prepare first timers.

Do correct the issue if I misjudged the situation.

Thanks for the patches! It seemed to me that the topics needed fairly extensive rewriting for grammar and clarity... Rather than provide suggestions in a comment here, I decided it would be more efficient to provide them in a new patch. Some notes:

a) The overview topic -- I changed the title and made it a lot shorter (it seemed repetitious).

b) Generally, many of the topics were not following the help topic standards, which has specific instructions for the Goal and Steps sections, how to describe navigation, etc.: https://www.drupal.org/docs/develop/documenting-your-project/help-topic-...

c) The Permissions topic wasn't written as a task, so I moved its information into the overview topic and removed this topic.

d) I did not make an interdiff file, because pretty much every line of the patch was different.

Hii
@jhodgdon thank you so much for reviewing patch. #43 looking good!

1. Patch applied. Help Topics module and Book module were enabled.
2. Reviewed Help Topics for grammar and spelling.
3. All links work!
4. Provided screenshots with all Help topics created!

All looks great to me!
Thanks!

Reviewing at Amsterdam2019

@alonaoneill already reviewed and I confirm.

Patch works just fine! Set to RTBC.

Just making sure the patch file is visible at the top of the issue (too many files were visible).

Oops. Actually I noticed a few of the Twig variables were not being used in some of the topics here. Redoing the patch slightly.

If someone wants to review the 2 topics that I updated in the interdiffs and make sure all the links still work, that would be helpful. Thanks!

This is looking great. A few notes.

1. book.about.html.twig:

I think we should mention which module provides this functionality. Since we are converting module overviews to topics, it's no longer explicit in the title of the page which module we're talking about (it's implicit/suggested, but not stated outright). So I think we should be clear and not make people guess. Even just slipping into current text would be fine. Something like:

Managing books

The topics listed below will help you create, edit, and configure books using Book.

Related topics

...

2. book.adding.html.twig:

A) In step 1, I think we should link to "Add content" (node/add).

B) Also in step 1, I think we should link "If you have configured additional content types that can be added to books" to the Configuring books topic (book.configuring.html.twig).

Great ideas! How's this?

Looks great! Thank you! Setting this to RTBC.

#52 looks good

+1 to it

#52 looks good! Thanks for Setting this to RTBC.

1. +++ b/core/modules/help_topics/help_topics/book.about.html.twig
   @@ -0,0 +1,26 @@
   +  <dt>{% trans %}Administer site configuration (in the System module section){% endtrans %}</dt>
   +  <dd>{% trans %}Allows users to configure books.{% endtrans %}
   +  </dd>
   

   Is the scope only to move content from hook_help() to the help topics? Or is the content also being improved at the same time? It seems irresponsible to say that "administer site configuration" is simply a permission that allows users to configure books.

   The content doesn't seem to be the same as the hook_help(), so I think we should expand the description here to explain that "administer site configuration" does more than that also, and is a restricted permission.

2. +++ b/core/modules/help_topics/help_topics/book.about.html.twig
   @@ -0,0 +1,26 @@
   +<p>{% trans %}If you have the core Book module installed, the topics listed below will help you create, edit, and configure books.{% endtrans %}</p>
   

   Does the help show up if the module is not installed?

3. +++ b/core/modules/help_topics/help_topics/book.adding.html.twig
   @@ -0,0 +1,20 @@
   +{% set node_add = render_var(url('node.add_page')) %}
   ...
   +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; <a href="{{ node_add }}"><em>Add content</em></a> &gt; <em>Book page</em>. If you have configured additional content types that can be added to books, you can substitute a different content type for <em>Book page</em> (see the <a href="{{ config }}">Configuring books</a> topic for more information).{% endtrans %}</li>
   
   +++ b/core/modules/help_topics/help_topics/book.configuring.html.twig
   @@ -0,0 +1,18 @@
   +{% set settings = render_var(url('book.settings')) %}
   ...
   +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Structure</em> &gt; <em>Books</em> &gt; <a href="{{ settings }}"><em>Settings</em></a>.{% endtrans %}</li>
   
   +++ b/core/modules/help_topics/help_topics/book.organizing.html.twig
   @@ -0,0 +1,19 @@
   +{% set overview = render_var(url('book.admin')) %}
   ...
   +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Structure</em> &gt; <a href="{{ overview }}"><em>Books</em></a>.{% endtrans %}</li>
   

   What if the user doesn't have permission to see the node add page?

   Same question about the book settings and admin links.

Sorry that it took so very long to provide a review here.

Good questions.

Regarding the URLs, we have an issue about it on the Roadmap actually:
#3090659: Make a way for help topics to generate links only if they work and are accessible
We have these types of links in other (committed) help topics already.... so my inclination is to leave the links as they are and fix them all on that other issue. See also
#2996305: Add support for the url() function to twig {%trans%}

Regarding the scope, the minimal plan is to make sure that information in hook_help is moved over, but the desired outcome is to have useful topics covering what each module or group of modules can do. So, I agree with your content improvement suggestions. I'll make a new patch sometime soon.

Here's a new patch -- again, thanks for the review!

Regarding the points in #57:

1. Added to the text some notes about the administer site configuration permission (it is VERY unfortunate, IMO, that the Book module doesn't have its own permission, but that is definitely out of scope for this issue).

2. Reworded the text so we still give the information that the core Book module is required, but you're right, they will not see this help unless it is already turned on. I think we still want to tell them which module provides the functionality, though, so they know not to turn it off.

3. As noted in the previous comment, I left the URLs as they were.

I have taken a quick look at the patch in #59

1. I would add if tasks affiliated with the book module have those mentioned security implications and in case which those are (maybe also a brief note that the Book module is missing its own permission cuz it is paired with other more security relevant settings - the reason for that warning). as an unexperienced user the mentioning of something like security implications with no explanation or context where to read up about it makes me insecure and sort of anxious to move on. and i wouldnt use a semicolon but create two sentences instead of one. improves the readability e.g.: "Allows users to do many site configuration tasks which includes configuring books. Some of those tasks have security implications."

2. That one is good and concise

and one nitpick and question. why has the "what are the permissions for books?" part not its own task topic?

on a side note the topics listed for top level help topics are sort of inconsistent. i guess the books help topic is the first covering permission with brief explanations (at least the first i have noticed it in yet). might be useful for the other top level help topics as well. that each top level help topic gets a task topic for permissions, configuration if applicable as a recommendation in the standards maybe?

The reason that the "what are the permissions" section is not its own task topic is that we don't want to have 25 topics about how to change permissions for each module. We want to have 1 topic about generically how to assign permissions to roles, and have various overview topics just list what the permissions are, and link to the topic about how to assign permissions (which at least when this patch was initially made, did not exist yet to link to). It does exist now, so we should link to it. I'll add that to the patch.

Regarding the inconsistency of what is top level, we are doing what we can for now, and then we have an issue to go back and update the outline and make sure the list of what is top-level and not top-level makes sense. The issue to reorganize is:
#2687107: Reorganize topics into sensible outline, and/or write more topics
We will also at that time be making sure cross-links are there to topics that didn't yet exist when we wrote other topics.

Regarding security implications, I would think that the ability to administer books in general would be something you wouldn't want to assign to every user on your site. Also that line about the security implications mirrors what you would see on the Permissions page for all the permissions that are sensitive:

Warning: Give to trusted roles only; this permission has security implications.

You get this same warning for the "administer nodes" permission in Node module, and many others.

Also it was requested in #57 item 1 to add this warning in. So I think it needs to stay. I don't really think it needs a lot of explanation here of why the ability to administer books is something you don't want to give to untrusted users.

Anyway, here's a new patch that will hopefully improve things a bit...

That way I haven't thought about it. 25 topics about changing permission doesn't make sense at all. You are right my bad! And the rewording you did is excellent.

Regarding the inconsistencies I've already written in the big pipe help topic issue. I got it and agree. :)

Committed and pushed c61c5dc139 to 9.1.x and c889f6b94d to 9.0.x and 04c05ee59a to 8.9.x. Thanks!

Backported to 8.9.x as this is an experimental module and only a text addition.