Background:
This issue is part of the task to update/create the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
- write the hook_help function
- review d.o. docs at https://drupal.org/documentation/modules/history

CommentFileSizeAuthor
#60 create-help-text-history-2029767-60.patch1.89 KBbatigolix
#60 interdiff.txt1.75 KBbatigolix
#58 interdiff.txt1.9 KBbatigolix
#58 create-help-text-history-2029767-58.patch1.89 KBbatigolix
#54 create-help-text-history-2029767-54.patch2.04 KBbatigolix
#54 interdiff.txt1.98 KBbatigolix
#51 interdiff-2029767-49-51.txt1.96 KBifrik
#51 create-help-text-history-2029767-51.patch1.97 KBifrik
#49 create-help-text-history-2029767-49.patch1.96 KBbatigolix
#45 create-help-text-history-2029767-45.patch1.96 KBbatigolix
#42 create-help-text-history-2029767-42_.patch5.59 KBbatigolix
#41 create-help-text-history-2029767-42.patch1.96 KBmarkconroy
#41 interdiff-2029767-40-42.txt0 bytesmarkconroy
#39 create-help-text-history-2029767-40.patch1.96 KBmarkconroy
#39 interdiff-2029767-30-40.txt0 bytesmarkconroy
#35 create-help-text-history-2029767-35.patch1.88 KBmarkconroy
#35 interdiff.2029767-31-35.txt4.98 KBmarkconroy
#32 create-help-text-history-2029767-30.patch4.86 KBmarkconroy
#32 interdiff-2029767-23-30.txt0 bytesmarkconroy
#31 create-help-text-history-2029767-30.patch4.86 KBmarkconroy
#31 interdiff-2029767-23-30.txt0 bytesmarkconroy
#23 create-help-text-history-2029767-23.patch1.86 KBmarkconroy
#23 interdiff-2029767-14-18.txt1.96 KBmarkconroy
#18 create-help-text-history-2029767-18.patch1.38 KBmarkconroy
#18 interdiff-2029767-14-18.txt1.96 KBmarkconroy
#16 interdiff.2029767-10-14.txt1.38 KBmarkconroy
#16 create-help-text-history-2029767-14.patch1.38 KBmarkconroy
#15 create-help-text-history-2029767-14.patch1.38 KBmarkconroy
#15 interdiff.2029767-10-14.txt1.38 KBmarkconroy
#12 create-help-text-history-2029767-10.patch1.29 KBbatigolix
#10 create-help-text-history-2029767-8.patch1.24 KBbatigolix
#8 create-help-text-history-2029767-8.patch1.24 KBbatigolix
#5 2029767-add-help-5.patch1.69 KBkscheirer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

batigolix’s picture

I cannot see where I can see this module in action. The module tracks node visits by users, but it is not clear if there is an UI.
See (#1120928: Move "history" table into separate History module)

catch’s picture

Priority: Normal » Critical
Marc DeLay’s picture

Starting on a patch for the hook_help function in entity.module
#midwest drupal summit

eek, wrong issue.

kscheirer’s picture

Assigned: Unassigned » kscheirer

I'll give it a shot

kscheirer’s picture

Status: Active » Needs review
FileSize
1.69 KB
kscheirer’s picture

Assigned: kscheirer » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

This help needs to follow our standards (http://drupal.org/node/632280).

batigolix’s picture

Component: documentation » history.module
Status: Needs work » Needs review
FileSize
1.24 KB

Rewritten to be more in line with the standards.

I set the component to history.module so that the maintainers can have a look at it as well

jhodgdon’s picture

Status: Needs review » Needs work

This help reads well and is clearly written. I'll let the maintainers of the module comment on its accuracy (for instance, is the "one month" expiration a setting or hard-wired?).

I noticed two small things that should, meanwhile, be fixed:

a) "The module does not provide a user interface For more information,..."
==> missing a . at end of sentence.

b) The link to the online docs is not following the standard at https://drupal.org/node/632280

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.24 KB

Patch fixes a) and b) from #9

Still pending: check for accuracy (see #9)

Status: Needs review » Needs work

The last submitted patch, create-help-text-history-2029767-8.patch, failed testing.

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

seems i attached the wrong patch :(

new attempt

tlyngej’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work!

Committed and pushed to 8.x. Thanks!

markconroy’s picture

Status: Fixed » Needs review
FileSize
1.38 KB
1.38 KB

Added link to views help page and edited link to history documentation page.

markconroy’s picture

added link to views help page and also edited link to history documentation page.

Status: Needs review » Needs work

The last submitted patch, create-help-text-history-2029767-14.patch, failed testing.

markconroy’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
1.38 KB

Fixed links (comma missing in array list) and some typos as well as wording to online documentation

Status: Needs review » Needs work

The last submitted patch, create-help-text-history-2029767-18.patch, failed testing.

ifrik’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, create-help-text-history-2029767-18.patch, failed testing.

ifrik’s picture

I've applied the patch in #18 - and it applied fine even though the test failed.
The wording appears accurate and according to the Help standard text. Since there is no user interface for this module, it does not require a "Uses" section.
The links work correctly, and they follow the new url() standard.
The link to the history module help page is listed on the general help page.

markconroy’s picture

Just realised (thanks ifrik) that I was patching a patch that had been committed to core.

Patch here is to core.

markconroy’s picture

Status: Needs work » Needs review

changing issue status

ifrik’s picture

I applied the patch and it works fine.
The wording is according to Standards, and appears to be correct.
The link to the Views help doesn't work yet, because that Views hook_help is still missing. But it goes to the right location.
The links are in the correct format.

Tests is still running, but if that passes then in can be marked as reviewed and tested

ifrik’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work!

Committed and pushed to 8.x. Thanks!

markconroy’s picture

Status: Fixed » Needs review

Super.

Thanks ifrik for you patience and webchick for committing - my first every commit/patch (I'll get moving on more now).

jhodgdon’s picture

Status: Needs review » Fixed

I'm assuming that was an inadvertent status change.

jhodgdon’s picture

Status: Fixed » Needs work

There is one minor typo -- very minor -- an extra space before the last sentence. A quick follow-up would be good? If we don't fix it now we will never be able to (it's a translated string and those get frozen).

markconroy’s picture

Status: Needs work » Needs review
FileSize
0 bytes
4.86 KB

I'm not sure exactly what extra space there was. I removed the one at the very end of the last sentence not before the last sentence.

markconroy’s picture

Not sure which space I was to remove. I removed the one at the end of the last sentence, not the one before it.

markconroy’s picture

Duplicate, sorry.

Not sure which space I was to remove. I removed the one at the end of the last sentence, not the one before it.

Status: Needs review » Needs work

The last submitted patch, create-help-text-history-2029767-30.patch, failed testing.

markconroy’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
1.88 KB

edited, re-submitting.

Status: Needs review » Needs work

The last submitted patch, create-help-text-history-2029767-35.patch, failed testing.

markconroy’s picture

Status: Needs work » Active

Can someone review that patch manually? It's working fine on my local machine.

Thanks.

jhodgdon’s picture

Status: Active » Needs work

You probably need to do a git pull to update your local repository. The patch does not apply for me either.

And by the way, the space that needs to be removed is here:

... a user interface.  For more ...

(between the . and For, there are two spaces instead of one)

markconroy’s picture

Status: Needs work » Needs review
FileSize
0 bytes
1.96 KB

I'll try again!

jhodgdon’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

Actually, I just noticed there are a couple of places in that line with two spaces in a row. The other one I just noticed was:

.... Views</a>  to show new or updated ...

Could you use your text editor to search for two spaces in a row in that text and remove all of them? Thanks!

Also, at this point it is not a critical issue, so devaluing the priority.

markconroy’s picture

Status: Needs work » Needs review
FileSize
0 bytes
1.96 KB

Should be good now.

batigolix’s picture

Think I annihilated the surplus spaces

jhodgdon’s picture

Status: Needs review » Needs work

Wait. This patch is supposed to just be removing extra spaces within the hook_help() in the history module. But the patch in #42 has all kinds of other stuff in it.

ifrik’s picture

@Mark Conroy:
The patch in #42 still has one last double space in it: Just before the "For more information..."
And if you are on it anyway: could you start the link to the online documentation earlier to include the whole phrase "online documentation for the History module" ?

@batigolix: There is indeed lots of other stuff in it, and also: the permissions got changed. That seems to happen more often - maybe you can change the setup? Or make interdiffs so that it's easier to spot?

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.96 KB

Sorry for that. This new patch *only* removes the double whitespace

ifrik’s picture

Status: Needs review » Reviewed & tested by the community

The link to the documentation page works, the link to views follows the default path pattern but currently breaks because the views module help page is missing.
I can't see any double spaces and the permissions are correct.

jhodgdon’s picture

Component: history.module » documentation

Thanks! Moving back to documentation so that I'll see it when I'm next doing commits.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Oh... Can we standardize the link to the on-line documentation so it matches our template? More should be inside the link text.

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.96 KB

Patch standardizes link text to docs

jhodgdon’s picture

Status: Needs review » Needs work

One more link accessibility issue. Can we move the link to Views to the first mention, and make sure that the link text is "Views module" not just "views"?

ifrik’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.97 KB
1.96 KB

I've changed the reference to Views module as proposed in #50

jhodgdon’s picture

This looks pretty good.

I noticed one more oddity. The module (as stated) as no UI itself. However, it says in the About that it "marks" content as New or Updated. Maybe terminology like "keeps track of" would be clearer -- to me "mark" sounds visible?

Actually, that whole sentence saying "by choosing Plain text as Format on the Manage Display page." is kind of redundant with the first sentence of About, isn't it? ... Oh, I see, I think it is describing the field and filter available to the Views module. Why not say it provides a field called xyz that shows ... and a filter called abc that does ... then?

And maybe we can move the last sentence about not providing a user interface to the beginning, such as "The module does not provide its own user interface, but instead makes its data available through the Views module"?

jhodgdon’s picture

Status: Needs review » Needs work
batigolix’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
2.04 KB

Attached patch addresses the points raised in #52

I could not find anything regarding

that whole sentence saying "by choosing Plain text as Format on the Manage Display page."

Was that a mistake, or am I missing something?

jhodgdon’s picture

Status: Needs review » Needs work

Sorry, yes, that was a copy/paste error on my part.

Here's what I actually meant to say. The help text currently says (it's all in one paragraph but I've broken it up into numbered sentences so I can refer to them:

1. The History module keeps track of which content a user has read.

2. Its data is available through the Views module.

3. The History module keeps track of content as new or updated depending on the last time the user viewed that content.

4. It also provides a filter to the Views module to show new or updated content.

5. History records older than one month will be removed during cron.

6. This means that content older than one month will always be considered read.

7. The module does not provide its own user interface, but instead makes its data available through the Views module.

8. For more information, see the online documentation for the History module.

So... This is very poor writing -- it's choppy (sentences are overly short) and quite redundant Sentences #2/#7 say the same thing. #1 is basically repeated in #3, and then kind of repeated again in #5, and then #6 repeats what #5 says mostly.

So maybe we could combine #1/3/5/6 into one sentence (or maybe two if entirely necessary) at the start, and then combine #2/4/7 into another sentence, and then keep #8 at the end?

catch’s picture

Title: Create hook_help for history module » Improve hook_help for history module

... so I don't keep wondering why this isn't critical.

jhodgdon’s picture

Good idea. :)

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
1.9 KB

Patch re-orders phrases as suggested in #55

jhodgdon’s picture

Status: Needs review » Needs work

Much better! Now all we need is for the link to the online docs to conform to our standard (put "online documentation ..." all into the link text).

batigolix’s picture

batigolix’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Issue tags: +Needs manual testing

Great! I think this one is ready for a quick manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

And since we didn't change the substance of the text here (just copy editing), I don't think we necessarily need to pass this into component history.module.

lostkangaroo’s picture

Status: Needs review » Reviewed & tested by the community

Manual test compete. All links function as expected and everything else appears to be in order.

jhodgdon’s picture

Thanks!

We're in the middle of a "major/critical commits only" week in Drupal Core, so this cannot be committed until Jan 23rd. I'll get to it then.

jhodgdon’s picture

Sigh. Now there is an issue tagged "avoid commit conflicts" which has a patch that touches history.module. Even though it probably does not have a conflict with this patch for people using a normal Git workflow, I'm being extra-careful lately about commit conflicts and will wait to commit this until
#1996238: Replace hook_library_info() by *.libraries.yml file
is finished.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

#1996238: Replace hook_library_info() by *.libraries.yml file is postponed atm so let's get this done. Committed 7833be2 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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