Use specific subject line in update emails

I am wondering if we should even send an email notification if there is a problem checking for updates.

Let's try this "Update notice for !site_name" for the subject line. Short sweet and generic.

I'm confused as to what a "System site" is. Maybe "Site update notice for...". Site names can start to get pretty long and subject lines are meant to be short and concise. http://www.businessweek.com/articles/2012-06-28/the-art-of-the-e-mail-subject-line says subject lines should be shorter than 50 characters. With "System site" addition that makes it 29 characters before the site name and to me !site_name is really important if I am administering a dozen or so Drupal sites. Thoughts?

Sounds good to me.

Re-rolling this patch with an addition that makes security updates (= UPDATE_NOT_SECURE or UPDATE_REVOKED for any module currently needing update) stand out by altering the subject to 'Security notice for !site_name' (d8) or 'Security update(s) available for !site_name' (d7).

D7 backport may not be accepted because string freeze, but attaching it anyway.

Note that the existing update_mail() appends ($message['subject'] .=) and this patch assigns. It's not clear from the existing code why it appended, and from my reading of drupal_mail() it would only be appending to empty string anyway, so AFAICT this is not a behaviour change.

fix D8 patch, set do-not-test on D7 patch.

Visually #10 looks very sensible - Can't imagine how it could be wrong.

But as identified in a (sprint) discussion - we can't imagine a practical way to TEST this exact situation - We would need two emails being generated with and without a 'security' level notice - given the state of any test system, and the state of the d.o update feeds.

Is this even possible? is it WAY too difficult, or can we just be sane and put this in as 'sensible'?

I can't locate any pre-existing unit tests that wrap the current functionality, so generally feel like this is good - but I can't prove it.

Reroll of #10.

Thanks for rerolling Amit!

Rerolling for reduced fuzz.

EDIT: Failing at Git. Ignore this :D

Rerolling for reduced fuzz :D

Needs re-roll.

re-roll

Ignore that patch, I left in testing code.

Postponing on #3276953: Add tests of new release emails

Converted latest patch to an MR and added tests.

The test no longer uses exact counts for the calls to the mocked functions because they vary so much now. If exact counts are wanted they would have to be added to the data provider array. That doesn't seem necessary to me for this test.

The test no longer uses exact counts for the calls to the mocked functions because they vary so much now.

Should a follow up be opened to add back in a different way for performance measuring? Seems like coverage we could be losing.

Ran the tests locally since I can't trigger test-only on the MR

Failed asserting that two strings are identical.
Expected :'Site notice for Test site'
Actual :'New release(s) available for Test site'

Change there seems fine so going to mark, but still wonder about a follow up.,

Hrm, "Site notice for @site_name" seems pretty unhelpful and very easy to ignore.

I'm in favor of adding "security" to the subject in the case of missing security updates, but the other change here seems like a step backwards, not forwards.

The original report was a complaint that it shouldn't say "New releases(s) available for @site_name" if there was a problem checking for updates. That could be fixed by checking the reason code (like we're now doing to detect missing security updates) and change the subject if it's an error to fetch.

Also, if this is really a bug, the title of this issue probably shouldn't be "improve update status email subject". 😅 If that's the scope of the issue, it should be a task, not a bug. And the new subject should actually be an improvement. 😂

IMHO, either we should focus on fixing the originally reported bug (that the subject shouldn't say "New release(s) available..." if there's a problem checking for updates), or we should change this to a task and make real improvements.

Sorry,
-Derek

@dww, thanks for the review.

I am leaving this as a bug since it is odd to get a message with a subject that releases are available when the body says that there was trouble getting the release information. I have updated the proposed resolution with my idea for handing this.

Couldn't run test-only feature but ran locally shouldn't "contrib, fetch fail" and "core fetch fail" fail also?

In this case they should not fail. Prior to this change there were no tests for fetching failures. These test are showing that the existing behavior does not change when there are both updates and fetch failures. That is, the email subject will always announce that new releases are available even if there are fetch failures for another project.

Thanks for following up #quietone. In that case believe this is good.

Thanks @smustgrave for the manual testing and question; @quietone's response helped me understand the logic better.

I've posted just a few small improvements to grammar on the MR, plus typehinted the rest of the test method. It's on the wobbly line for scope but we're allowed to break test hint signatures at any point, and it helped me understand the data provider structure for reviewing all the new cases. (Plus it will save work down the road.)

Once those are fixed up I think this is ready.

Saving issue credits.

Thanks @pradhumanjain2311 for your contributions. The point of suggestions is for the existing contributors to the issue to have a chance to +1 the reviewer's suggested changes. A person who has not otherwise contributed to the issue just clicking buttons to apply the suggestions does not really help move the issue forward. Therefore, I won't be crediting this contribution. In the future, you can receive credit by providing a review of an issue with open questions.

Marking NR for one of the issue's contributors to review the applied suggestions.

Does appear as suggestions were applied.

@smustgrave, again, I don't need validation that the suggestions were applied -- I can see that from the tooling -- I need validation that they were good suggestions. Thanks! :)

Left comments on the threads

3 agree, 1 I'm 50/50, 1 I made a suggestion.

The 1 suggestion can be done probably directly from gitlab but didn't move to NW @xjm agree on it?

Moving to NW for the 1 suggestion, which seems like a good task. Also could use a rebase being 300+ commits behind.

I have resolved merge conflicts and rebased it please have a look.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I fixed most of the issues reported by the Queue Bot, but there's still a PHPSTAN issue to fix: https://git.drupalcode.org/issue/drupal-1818764/-/jobs/5007233

 ------ --------------------------------------------------------------------- 
  Line   core/modules/update/update.module                                    
 ------ --------------------------------------------------------------------- 
  207    Function _update_manager_access not found.                           
         🪪  function.notFound                                                
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  

Where is this function _update_manager_access implemented? was it ever implemented in this MR? I think it's a legacy function from D7 that needs to be refactored here:

/**
 * Access callback: Resolves if the current user can access updater menu items.
 *
 * It both enforces the 'administer software updates' permission and the global
 * kill switch for the authorize.php script.
 *
 * @return
 *   TRUE if the current user can access the updater menu items; FALSE
 *   otherwise.
 *
 * @see update_menu()
 */
function update_manager_access() {
  return variable_get('allow_authorize_operations', TRUE) && user_access('administer software updates');
}

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I resolved the issue that the review bot raised however there is the nightwatch test failing. Unsure if its just glitching and a rerun is fine, or if its because we're behind on commits?

@nexusnovaz, A failing Nightwatch test is not related to the change here and is likely one of the random fails that exist. You can either re-run the test or add a comment that the failed tests are unrelated and then set the issue to 'needs review'.

Feedback appears to be addressed on this one.

This is sort of disruptive in its way (it will start bypassing existing mail filters), so let's have a change record for it. It should also be committed to 11.x only. Thanks!

This is getting close. Pushed a few commits to resolve a few more of the open threads. Down to 1 (the request for inline comments describing each case in UpdateMailTest::providerTestUpdateEmail()).

Also, opened #3538637: Fix problems with UpdateMailTest: only test valid cases, use correct paths, remove dead code which I noticed while reviewing this. That's totally out of scope here, but should happen.

Initial CR: https://www.drupal.org/node/3538659

Okay, did some more sleuthing, and decided that 2 of the existing cases in this test are totally bogus, as is 1 of the new ones we added:

• _update_cron_notify() is the only place sends the 'status_notify' email.
• It returns early if there are no values in the $params array.
• It therefore seems pointless to unit test our hook_mail() implementation without parameters.

Removed all 3 cases, and the related @todo comments from my attempt to document everything.

Apologies for the possible scope creep. Perhaps we should postpone this on a new follow-up just to fix all the brokenness in this test, then circle back to fix the bug and expand the test. But it seemed more prudent to fix them all in 1 shot.

With the draft CR done, all MR threads resolved, and a green pipeline, this is ready for review again. 😅

Per Slack chat with @xjm, postponing this bug fix on #3538637: Fix problems with UpdateMailTest: only test valid cases, use correct paths, remove dead code, which I've re-scoped into fixing UpdateMailTest before we try to expand it here.

Anyone following this issue: #3538637: Fix problems with UpdateMailTest: only test valid cases, use correct paths, remove dead code is now ready for review if anyone is willing to give it a look. Thanks!

Trying to make progress while we wait for #3538637: Fix problems with UpdateMailTest: only test valid cases, use correct paths, remove dead code to land.

The old branch had massive conflicts when trying to rebase it on top of #3538637. So I opened a new branch, added a single commit for #3538637, then 2 commits for the merged results of that and the previous MR branch, 1 for the change to set the subject, one for the test coverage. Once 3538637 is in 11.x, it'll be easy to interactive rebase this to remove that 1 commit, then we'll have an easy 2 commits here to review...

The blocker, #3538637: Fix problems with UpdateMailTest: only test valid cases, use correct paths, remove dead code, is now in 11.x. I rebased MR 12950 to clean that up. This should be ready for review now.

Setting to needs work for the @todo and the follow-up (see MR comments).

Made the follow-up: #3553063: _update_message_text() calls Url::fromRoute without options.

How about this for the @todo:

diff --git a/core/modules/update/update.module b/core/modules/update/update.module
index 1a217fa54da..b492d4f6ba3 100644
--- a/core/modules/update/update.module
+++ b/core/modules/update/update.module
@@ -214,6 +214,9 @@ function _update_message_text($msg_type, $msg_reason, $langcode = NULL) {
     case UpdateFetcherInterface::NOT_CHECKED:
     case UpdateFetcherInterface::NOT_FETCHED:
     case UpdateFetcherInterface::FETCH_PENDING:
+      // @todo Conditionally generate absolute URL when used in email context
+      // and pass language option to Url::fromRoute().
+      // @see https://drupal.org/i/3553063
       if ($msg_type == 'core') {
         $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [':update-report' => Url::fromRoute('update.status')->toString()], ['langcode' => $langcode]);
       }