Problem/Motivation

Noticed as part of #2501683-46: Remove SafeMarkup::set in _update_message_text() the words "available updates" linking to two different pages in the same paragraph. From a usability perspective two links with the same label should go to the same page. This is not the case. Check the screenshot:

Status report page with confusing language

steps to reproduce

  1. install d8 head using Git
  2. log in as admin
  3. go to admin/reports/status

Proposed resolution

Simply remove the second sentence.

  • Since there was a problem checking available updates, it doesn't make sense to tell people to try to install them.
  • That sentence was never intended to be displayed in this scenario anyway.
  • The link we are keeping goes to /admin/reports/updates/update, the "List" tab of the Update page, making it easy to go to the "Update" tab, which is where the removed link went.

See comment #122 for screenshots.


Remaining tasks

  • Waiting on usability review from yoroy
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions DONE
Manually test the patch Novice Instructions DONE
Embed before and after screenshots in the issue summary Novice Instructions DONE
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions DONE

User interface changes

No new patterns.

API changes

No.

Data model changes

No.

CommentFileSizeAuthor
#130 update-status-2545520-122.patch2.42 KBxjm
#130 update-status-2545520-122.patch2.42 KBxjm
#126 update-status-2545520-126-do-not-test.patch1.94 KBbenjifisher
#123 UPDATE_UNKNOWN-update-manager.png32.36 KBDavid_Rothstein
#123 UPDATE_UNKNOWN-available-updates.png73.54 KBDavid_Rothstein
#123 UPDATE_NOT_FETCHED-update-manager.png32.35 KBDavid_Rothstein
#123 UPDATE_NOT_FETCHED-available-updates.png71.52 KBDavid_Rothstein
#123 UPDATE_NOT_CHECKED-update-manager.png54.26 KBDavid_Rothstein
#123 UPDATE_NOT_CHECKED-available-updates.png101.35 KBDavid_Rothstein
#123 UPDATE_FETCH_PENDING-update-manager.png32.14 KBDavid_Rothstein
#123 UPDATE_FETCH_PENDING-available-updates.png71.26 KBDavid_Rothstein
#122 status-report-when-updates-are-available.png26.12 KBDavid_Rothstein
#122 UPDATE_FETCH_PENDING-update-manager.png30.54 KBDavid_Rothstein
#122 UPDATE_FETCH_PENDING-available-updates.png60.82 KBDavid_Rothstein
#122 UPDATE_FETCH_PENDING-status-report.png15.33 KBDavid_Rothstein
#122 UPDATE_NOT_CHECKED-update-manager.png47.43 KBDavid_Rothstein
#122 UPDATE_NOT_CHECKED-available-updates.png81.73 KBDavid_Rothstein
#122 UPDATE_NOT_CHECKED-status-report.png16.54 KBDavid_Rothstein
#122 UPDATE_NOT_FETCHED-update-manager.png30.88 KBDavid_Rothstein
#122 UPDATE_NOT_FETCHED-available-updates.png61.42 KBDavid_Rothstein
#122 UPDATE_NOT_FETCHED-status-report.png15.72 KBDavid_Rothstein
#122 UPDATE_UNKNOWN-update-manager.png30.87 KBDavid_Rothstein
#122 UPDATE_UNKNOWN-available-updates.png62.47 KBDavid_Rothstein
#122 UPDATE_UNKNOWN-status-report.png15.65 KBDavid_Rothstein
#122 update-status-2545520-122.patch2.42 KBDavid_Rothstein
#111 change_words_so_that-2545520-110.patch2.09 KBlukas.fischer
#111 after-patch-#110.png60.22 KBlukas.fischer
#111 before-patch.png69.84 KBlukas.fischer
#108 On click to manual updates - After patch.jpg85.11 KBNikitaJain
#108 After applying the patch in #101.jpg131.71 KBNikitaJain
#108 before applying patch.jpg113.82 KBNikitaJain
#105 update-page.png52.74 KBrovo
#105 available-updates-page.png45.75 KBrovo
#103 interdiff-2545520-88-94.txt1.06 KBmradcliffe
#102 proposed_patch_2545520-93.png29.55 KBrovo
#101 change-links-2545520-94.patch1.16 KBrovo
#101 proposed_patch_2545520-93.png29.68 KBrovo
#101 after_patch_2545520-88.png30.26 KBrovo
#88 after_patch_2545520.png208.65 KBa11y.matters
#88 before_patch_2545520.png229.51 KBa11y.matters
#88 change-links-2545520.patch1.17 KBa11y.matters
#80 change-words-2545520-80.patch1.07 KBtar_inet
#80 after-patch-2545520.png84.34 KBtar_inet
#80 before-patch-2545520.png89.19 KBtar_inet
#77 Available updates - on status reports.jpg87.25 KBNikitaJain
#71 patch-verify-70-status-report.jpg31.06 KBrosschive
#70 change-words-so-2545520-70.patch1.09 KBmgifford
#61 after patch - Screen Shot 2017-01-28.png79.78 KBsteverossnyc
#61 before patch - Screen Shot 2017-01-28 .png82.98 KBsteverossnyc
#60 change-words-so-2545520-60.patch2.35 KBBarisW
#59 change-words-so-2545520-59.patch1.09 KBBarisW
#58 change-words-so-2545520-58.patch2.01 KBBarisW
#54 interdiff-2545520-51-54.txt1.18 KBowenpm3
#54 change-words-so-2545520-54.patch3.79 KBowenpm3
#51 after.png35.75 KBowenpm3
#51 before.png44.37 KBowenpm3
#51 change-words-so-2545520-51.patch3.5 KBowenpm3
#45 available-updates-after.jpg17.41 KBvulcanr
#45 available-updates-before.jpg28.91 KBvulcanr
#42 After appying patch - Available updates.png52.24 KBNikitaJain
#41 Link - Available updates.png42.79 KBNikitaJain
#41 Version 8.1.7.png44.71 KBNikitaJain
#41 Version 8.2.png40.59 KBNikitaJain
#41 Version 8.3.png47.01 KBNikitaJain
#38 change_words_so_that-2545520-38-after.png65.98 KBAnicky
#38 change_words_so_that-2545520-38-before.png68.41 KBAnicky
#38 change_words_so_that-2545520-38.patch2.3 KBAnicky
#25 afterApplyingPatch.png110.27 KBshwetaneelsharma
#25 originalIssue.png152.94 KBshwetaneelsharma
#24 available-updates-link-2545520-24.patch2.04 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch available-updates-link-2545520-24.patch. Unable to apply patch. See the log in the details link for more information. View
#24 interdiff-20-24.txt1.53 KBhussainweb
#20 available-updates-link-2545520-20a.patch2.35 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,458 pass(es). View
#15 available-updates.png35.02 KBDavid_Rothstein
#15 available-updates-link-2545520-15.patch2.12 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,006 pass(es). View
#11 available-updates-link-2545520-11.patch2.16 KBsdstyles
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,925 pass(es). View
#11 user_cant_make_updates.png57.14 KBsdstyles
#11 admin_status_report.png61.45 KBsdstyles
#8 Update page.png101.58 KBxlin
#8 List page.png243.79 KBxlin
#8 screenshot.png49.53 KBxlin
#8 2545520-8-change-wording-for-available-update.patch2.15 KBxlin
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,833 pass(es). View
#7 available-updates-link-2545520-7.patch1.37 KBsdstyles
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,835 pass(es). View
#3 availaible-updates-links.png167.33 KBsdstyles
#3 available-updates-link-2545520-3.patch1.01 KBsdstyles
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,826 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

YesCT’s picture

Issue tags: +Novice

code to change might be in core/modules/update/update.install if #2501683: Remove SafeMarkup::set in _update_message_text() gets in first.

or it might still be in core/modules/update/update.module if that takes a bit.

xlin’s picture

Assigned: Unassigned » xlin
sdstyles’s picture

Issue summary: View changes
FileSize
1.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,826 pass(es). View
167.33 KB

Changed available updates link

sdstyles’s picture

Status: Active » Needs review
xlin’s picture

Assigned: xlin » Unassigned
mgifford’s picture

@sdstyles is that going to ensure that if ($msg_type == 'core') { is going to give you the same results as:

  if ($report_link) {
    if (update_manager_access()) {

Might it be better to change the text so that the link text is differentiated:
<a href="@update-report">available updates report</a>
<a href="@available_updates">available updates</a>

It still seems like available updates could have duplicate links going to different places with that patch, but I could be wrong about that.

@xq1003 are you working on a solution too?

sdstyles’s picture

FileSize
1.37 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,835 pass(es). View

@mgifford I've created another patch to fix wrong path. I added a check to verify if user has access to manage updates.
Users with access have the link to Update page (admin/reports/updates/update), users without access can only see Reports page (admin/reports/updates).
These changes are applied for both Core and modules messages.
I thought to rename @update-report link and keep path to report page, but for second case (user will have to different links with same path).

xlin’s picture

FileSize
2.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,833 pass(es). View
49.53 KB
243.79 KB
101.58 KB

@mgifford @sdstyles, I take a different approach, please correct me if I'm wrong.

Instead of linking user to admin/reports/updates/update for both links, maybe we can change the wording for the first link. Something like

<td>
  <div class="description">There was a problem checking <a href="/drupal/admin/reports/updates">list of available updates</a> for Drupal. See the <a href="/drupal/admin/reports/updates/update">available updates</a> page for more information and to install your missing updates.</div>
</td>

The "List" page contains more information than "Update" page, could be more useful.

mgifford’s picture

@xq1003 I think you've got the better approach. If the two links go to different places, they need different text.

I think it needs a "the" in the text though:
There was a problem checking the <a href="/drupal/admin/reports/updates">list of available updates</a> for Drupal.

Now the Available updates page admin/reports/updates is indeed a list of available updates.

The Update page admin/reports/updates/update is where you can actually apply the updates to contributed modules.

Isn't this more accurate for the 2nd sentence:
Visit the <a href="/drupal/admin/reports/updates/update">update</a> page to apply updates to your contributed modules.

sdstyles’s picture

@mgifford I agree with first link, but for the second
Visit the <a href="/drupal/admin/reports/updates/update">update</a> page to apply updates to your contributed modules.
it's ok that this message to be displayed for "Drupal core update status" too? not only for "Module and theme update status"?

sdstyles’s picture

FileSize
61.45 KB
57.14 KB
2.16 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,925 pass(es). View

@mgifford a new patch. You can see in screenshots the message displayed for administrator, and the message when user has no access to make updates.

xlin’s picture

@mgifford @sdstyles , I agree with both of you on the firs link.

For the second link, maybe we can try something like

Visit the <a href="/drupal/admin/reports/updates/update">update</a> page for more information and to install your missing updates.

This way, it don't sounds too specific and can be apply to both core and contributed module.

mgifford’s picture

In the patch in #11, a full administrator will see:
There was a problem checking the <a href="/admin/reports/updates">list of available updates</a> for Drupal. See the <a href="/admin/reports/updates/update">available updates</a> page for more information and to install your missing updates.

and if the user can't make updates they see:
There was a problem checking the <a href="/admin/reports/updates">list of available updates</a> for Drupal. Visit the <a href="/admin/reports/updates">update</a> page to see updates to your contributed modules.

It's not as bad as it was, but having two different pieces of text link to the same URL is also not very good. Why would we repeat the link in the first place?

I think this would make more sense to a full administrator:
New software upgrades are available from the Drupal.org repository. A list of projects with upgrades is available from the <a href="/admin/reports/updates">available updates</a> page. To apply upgrades from within the administration interface go to the <a href="/admin/reports/updates/update">available updates</a> page to apply the latest release.

and if the user can't make updates they see:
New software upgrades are available from the Drupal.org repository. A list of projects with upgrades is available from the <a href="/admin/reports/updates">available updates</a> page. Contact your Drupal administrator about applying these upgrades.

Actually, why return anything at all in the second sentence if the user doesn’t have the authority to address updates.

This shouldn't be all that complicated an issue. Trying to keep it super general probably isn't helping us here.

Sentence 1 is informing users that there is a list of updates available (Different links with different text.). Sentence 2 simply tells administrators where they can go to apply updates within the admin interface (Just one link).

jhodgdon’s picture

I like the direction of #13... The 2nd "if they can't" paragraph I think is good. But the proposed "if they can" pgae goes back to the original problem being reported here, that there are two links in the paragraph both called "available updates", that go to two different places, which is (right?) not good for accessibility.

How about something like this:

New software upgrades are available from the Drupal.org repository. You can view a list of projects with upgrades on the <a href="/admin/reports/updates">available updates report</a> page, or you can <a href="/admin/reports/updates/update">apply updates using the user interface</a>.
David_Rothstein’s picture

FileSize
2.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,006 pass(es). View
35.02 KB

I'm not sure how those suggestions would work given that this message displays when the update status can't be determined?

The current behavior is wrong in a lot of ways. The first link is displayed when the calling code specifically requests that it not be displayed (and since the calling code sometimes puts this text in emails, including a link with a non-absolute URL would be bad). The second link duplicates the first (for low-level admins) or makes no sense at all (for high-level admins) - we shouldn't be telling people to go somewhere to "install your missing updates" when we don't know of any updates that are actually missing.

So I think we should just remove the bad links and leave the one good one behind.

The text would be the same for either type of user in this case (see attached screenshot), and the single "available updates" link points to admin/reports/updates only.

David_Rothstein’s picture

Issue tags: +needs backport to D7

Also tagging this for backport - especially if it's an accessibility issue it's worth changing this admin-facing text to fix it.

David_Rothstein’s picture

I noticed that this patch more or less makes it go back to how it was in Drupal 6. This is called "progress" :)

Which made me wonder why it was changed in Drupal 7. Git history points to #639434: Update error messages but if you look at the discussion there, it looks like it was just a mistake. Bojhan (in comment #3) was actually proposing to change the entire text to:

There was a problem checking [available updates] for Drupal.

which is along the lines of the text in my above patch (but even shorter).

But the committed patch in that issue didn't do anything to keep the second sentence from being appended to the first... hence it is still appended.

We could actually just go with Bojhan's text here instead. I actually thought about that independently while writing the above patch, but figured it might be a little too short.

mgifford’s picture

I think short is good. Redundant is bad. @David_Rothstein you made a lot of good points here.

The simple approach from @Bojhan seems fine to me. I wonder if "There was a problem checking [available updates] for Drupal.org." would be a bit clearer for folks who just know that they are using Drupal and might be confused how that could be possible.

We'd just have one sentence & one link. Fits well with the approach we're aiming for in D8 in general.

And we'll just nix the whole section within:

   if ($report_link) {
    if (update_manager_access()) {}}

@jhodgdon I think I left in the wrong link in #13, thanks for catching that. What do you think of simplifying it further to be just one sentence & link as suggested in #15 & #17?

jhodgdon’s picture

Status: Needs review » Needs work

Go for it!

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,458 pass(es). View

@David_Rothstein, @Bojhan's text which includes a link, as I read it, but think you had the right approach of removing it.
There was a problem checking [available updates] for Drupal.

I also think that there really is no reason to differentiate between Core & modules & themes here. It's not like an available update for a module/theme is going to be any more/less important than a Core update necessarily. The logic really doesn't add much value. If there's a problem checking for updates, there's a problem checking for updates.

Breaking up the two sections to alert users to problems checking for updates and providing a link to administrators made more sense to me the longer I looked at it so I brought over the $skip_update_manager.

In this patch there will be one sentence for everyone who sees the report & two sentences (and a link) for administrators.

jhodgdon’s picture

Looks reasonable to me, mostly... a couple of small things:

  1. +++ b/core/modules/update/update.module
    @@ -516,12 +517,11 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    -      if ($msg_type == 'core') {
    -        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
    -      }
    -      else {
    -        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
    -      }
    +      $text = t('There was a problem checking available updates for Drupal.', array(), array('langcode' => $langcode));
    +
    

    It looks like we probably do not need the $msg_type local variable any more? Not sure, but worth checking on...

  2. +++ b/core/modules/update/update.module
    @@ -516,12 +517,11 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      $text = t('There was a problem checking available updates for Drupal.', array(), array('langcode' => $langcode));
    

    Why do we need "for Drupal" here at all, if this is going to be for both core and contrib?

David_Rothstein’s picture

I don't understand why we'd remove the link to admin/reports/updates? That's the page that gives you a bit more info about what the problem checking available updates actually was, so I'd think we want people to be able to go there...

Also if I'm reading the patch right, it will remove the link in all cases (not just when there are problems checking updates). That definitely doesn't look right to me. For administrators who don't have access to the Update Manager - or for entire sites that don't use the Update Manager at all - admin/reports/updates is the page they go to in order to see all the details about what modules and themes actually need updating.

I also think that there really is no reason to differentiate between Core & modules & themes here.... If there's a problem checking for updates, there's a problem checking for updates.
.....
Why do we need "for Drupal" here at all, if this is going to be for both core and contrib?

I like that combination a lot :) Just saying "There was a problem checking available updates" and leaving it at that sounds perfect to me (as long as there is a link).

jhodgdon’s picture

Status: Needs review » Needs work

Needs work then...

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
2.04 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch available-updates-link-2545520-24.patch. Unable to apply patch. See the log in the details link for more information. View

Fixing for comments in #21.2. Regarding point 1, the variable $msg_type is actually a parameter, not a local variable and is used in other case blocks.

Regarding #22, I added the earlier message as pointed out that there must be some way to go to the updates page from here, even if the update check failed here.

shwetaneelsharma’s picture

Status: Needs review » Needs work
FileSize
152.94 KB
110.27 KB

I applied the patch on an older version so that I can see the available updates messages. This was accomplished by the command git reset HEAD~500 --hard
As expected, I could see the available updates message. Screenshot attached(originalIssue.png)
After applying the patch in comment#24 using the command git apply -v available-updates-link-2545520-24.patch the message changed. Text "Drupal" has been removed from the message."There was a problem checking available updates". Attaching a screenshot after applying the patch (afterApplyingPatch.png)
Just to double check I reverted the patch using git reset HEAD~1 --hard and the original message was visible.

shwetaneelsharma’s picture

Status: Needs work » Reviewed & tested by the community
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

So... I thought in the discussion above, that people thought we *should* have a link still to both the original updates report, as well as the page that lets you actually perform updates?

It seems like with this patch, if you don't have your site set up so people can do updates from the UI, you don't have any alternative. You still need to see the available updates report so you can see what is out of date, right?

minhein’s picture

Status: Needs review » Needs work

I am working on it this issue from the Barcelona DrupalCon. After words with my mentor, jp.stacey, we need some more works on it according to the commented by jhodgdon.

The last submitted patch, 24: available-updates-link-2545520-24.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drnikki’s picture

Assigned: Unassigned » drnikki
drnikki’s picture

After reading over this issue a few times, I wanted to clarify the direction before making a patch.

It sounds like, to @jhodgdon and @David_Rothstein's points, we want to include a link to the update report for people who can't do updates via the UI, but still fix the issue of 'update' linking to two different destinations. How about:

There was a problem checking for updates.

And then for administrators to see:

See the available updates page for more information and to install your missing updates.

@mgifford, @jhodgdon - thoughts?

jhodgdon’s picture

For accessibility, link text should tell you where you are going. "a problem" doesn't tell you where you are going, so it is not great link text. So, the first sentence needs a rewrite so the link text can be something like "updates report" or similar.

David_Rothstein’s picture

What's wrong with the simple solution discussed above?

Just have a single sentence, with the following text:

"There was a problem checking available updates."

with the link pointing to admin/reports/updates.

(Linking to admin/reports/updates/update on top of that seems unnecessary since if there was a problem checking updates there shouldn't be an actual update to perform, right? And in any case, the text above this paragraph - i..e the "Unknown release date" text in the screenshot in the issue summary - links there already.)

jhodgdon’s picture

I looked over the discussion above, and you're right! I think there was consensus for this simple solution. Sorry for the misdirection in my previous comment. Please ignore #27 and go with #35.

Anicky’s picture

Assigned: drnikki » Anicky
Issue tags: +DevDaysMilan
Anicky’s picture

As @David_Rothstein said, I removed "for Drupal" in the sentence "There was a problem checking available updates." and I removed the other sentence "See the available updates...".

Also, I saw this in update.module (line 444):

function update_mail($key, &$message, $params) {
  ...
  foreach ($params as $msg_type => $msg_reason) {
    $message['body'][] = _update_message_text($msg_type, $msg_reason, $langcode);
  }
  $message['body'][] = t('See the available updates page for more information:', array(), array('langcode' => $langcode)) . "\n" . \Drupal::url('update.status', [], ['absolute' => TRUE, 'language' => $language]);
  ...
}

Does the sentence "See the available updates page..." have to be removed as well?

Anicky’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue summary: View changes
Status: Needs work » Needs review

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

NikitaJain’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
47.01 KB
40.59 KB
44.71 KB
42.79 KB

Tested and verified on an older version (8.1.7) so that I can see the available updates message. After applying the patch #38 change_words_so_that-2545520-38_0.patch the message changed to "There are updates available for your version of Drupal. To ensure the proper functioning of your site, you should update as soon as possible."
Screenshots attached.

NikitaJain’s picture

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
  1. Those screenshots (before vs. after) show that the patch is removing the text in all scenarios, even where there is no duplicate link text.

    Earlier patches such as #15 only removed it in the "there was a problem checking available updates" scenario specifically discussed in this issue.

    Perhaps this makes sense but I'm not sure. ("Out of date" and "available updates" do link to the same place, so I guess there's an argument to be made that the latter is unnecessary here too.) Needs discussion at any rate.

    Also if you're going ahead with that then the whole code comment visible at the top of the patch file (about "format the two translated strings together") no longer makes sense and needs to be changed.

  2. Also, I saw this in update.module (line 444):

    function update_mail($key, &$message, $params) {
      ...
      foreach ($params as $msg_type => $msg_reason) {
        $message['body'][] = _update_message_text($msg_type, $msg_reason, $langcode);
      }
      $message['body'][] = t('See the available updates page for more information:', array(), array('langcode' => $langcode)) . "\n" . \Drupal::url('update.status', [], ['absolute' => TRUE, 'language' => $language]);
      ...
    }
    

    Does the sentence "See the available updates page..." have to be removed as well?

    Good question. Seems like it could be removed also (for consistency), but only in the case where it's a duplicate link that already appears elsewhere in the email. Which again seems like it would be limited to the "there was a problem checking available updates" case.

Anicky’s picture

Assigned: Anicky » Unassigned
vulcanr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
28.91 KB
17.41 KB

I just tested this patch and changes looks good as per the expectation in the description.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

There are unanswered items from the review in #43.

owenpm3’s picture

In Drupal-8.3.x-dev (which is what this issue's currently marked as) I get the following:

Drupal core update status	Out of date (version 8.2.5 available)
There are updates available for your version of Drupal. To ensure the proper functioning of your site, you should update as soon as possible. See the available updates page for more information and to install your missing updates.

And:

Module and theme update status	Out of date
There are updates available for one or more of your modules or themes. To ensure the proper functioning of your site, you should update as soon as possible. See the available updates page for more information and to install your missing updates.

Has this issue been corrected elsewhere? Because there's now only the one available updates link showing. If so, does it just need to be backported to Drupal 7?

mgifford’s picture

David_Rothstein’s picture

@omorrill I don't think the duplicate link ever happened in that case - only in the "There was a problem checking available updates" scenario shown in the issue summary and discussed above.

So I guess in practice this actually doesn't occur much for anyone -- it may be most common in practice for developers running a local Git checkout, which doesn't have a release date in the .info.yml files (Drupal 8) or .info files (Drupal 7).

David_Rothstein’s picture

Restoring accidentally deleted tags.

owenpm3’s picture

Status: Needs work » Needs review
FileSize
3.5 KB
44.37 KB
35.75 KB

Thanks, @David_Rothstein for helping me clear that up. I've removed the duplicate line so that it's now just the following:

There was a problem checking available updates.

And "available updates" is a link.

I also added the logic you had in your #15 patch to apply only on the "There was a problem checking..." item. I added that same logic to the line in the email body.

I believe that covers what was talked about in #43.

Status: Needs review » Needs work

The last submitted patch, 51: change-words-so-2545520-51.patch, failed testing.

vulcanr’s picture

Tested patch #51 locally, works fine. Would be nice to scope the failure and resubmit it.

owenpm3’s picture

Update.install did not like that $skip_update_manager was undefined. I believe I've solved that problem--check interdiff.

owenpm3’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 54: change-words-so-2545520-54.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

BarisW’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

Can we simplify it to this?

There was a problem checking available updates for Drupal. See the available updates page for more information.

Thus, simply removing the link from the first sentence?

BarisW’s picture

Apologies, it should be this patch.

BarisW’s picture

Sorry for making a mess of this thread. I now understand what owenpm3 was trying to achieve. This patch should implement that. Basically, we don't distinct /update and update/update. We always link to /update. By removing the link to this page from the first part of the sentence, I believe we fixed the issue.

steverossnyc’s picture

Applied the patch and it solves the issue.
Before patch:before
After patch: after

arkiii’s picture

Applied the patch and checked code and all looks good to me.

mgifford’s picture

@steverossnyc do you have concerns with marking it RTBC when it goes green?

steverossnyc’s picture

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

Uhhh... How am I supposed to test something with the latest version and get an Update message for a newer version? I don't get it ;-) Any clue is welcome!

mgifford’s picture

@djouuuuh it is RTBC now, so no need to test it. Fine to re-test it of course.

Would be comparing:
https://simplytest.me/project/drupal/8.4.x

sith:
https://simplytest.me/project/drupal/8.4.x?patch[]=https://www.drupal.or...

djouuuuh’s picture

@mgifford Ohhhh you just made my day showing me it was actually possible to apply a patch on a fresh Drupal sandbox with Simplytest.me... I'm so happy I want to cry :-D

mgifford’s picture

Perfect. Happy to help @djouuuuh.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

This patch results in different text than previously discussed. Why wasn't this done like shown in #45?

It also removes the "and to install your missing updates" phrase entirely, even when there are missing updates. While I'm not sure that phrase is critically important, I don't see an obvious reason to remove it here either... Again, I think what we need to do in this issue is as discussed in #43 - only modify text for the "there was a problem checking available updates" scenario.

mgifford’s picture

@BarisW proposed it in #58. Simply removing the link from the first sentence resolves the accessibility problem without loosing meaning for the user.

This just takes the last half of the patch in #60, so it will include the "and to install your missing updates" phrase and the logic behind it.

rosschive’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
31.06 KB

Taking a look as part of Global Sprint Weekend (Boston)...

Following creation of a sandbox site w/ @mgifford 's patch (via simplytest.me), verifying that the first instance of the "available updates" hyperlink has been removed (i.e., navigating to Admin > Reports > Status report). Remaining "available updates" hyperlink resolves as expected.

patch verification screenshot

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

But the problem with that was already discussed earlier in the issue, wasn't it? It removes the most useful link (admin/reports/updates) and leaves behind the one that makes much less sense in this context (admin/reports/updates/update).

Also while we're already changing the translated string, it would be nice to improve the wording as the other patches did... it's not strictly related to this issue but it's how it was always supposed to be and it avoids more broken translations (and more work for translators) to change it again in the future.

BarisW’s picture

I see that Mike's patch misses an important part of my patch, namely changing the link in the second sentence to the status overview page.

mgifford’s picture

Sorry @BarisW & @David_Rothstein.. Hopefully someone can revise this and we can get this simple patch RTBC'd for a final time.

tameeshb’s picture

Assigned: Unassigned » tameeshb

I'll revise the patch! :)

David_Rothstein’s picture

I see that Mike's patch misses an important part of my patch, namely changing the link in the second sentence to the status overview page.

But then the link will be less useful in the case where there actually are new updates to install.

I really think this issue should go back to the earlier way it was doing it.

NikitaJain’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
87.25 KB

Verified on 8.4.x-dev branch without applying the latest patch on it. The issue is already fixed on 8.4.x-dev branch.
- Navigate to /admin/reports/status
- Check the 'Drupal Core Update Status'
- The first hyperlink for the "available updates" has been removed.
- Its appearing like this "There is a security update available for your version of Drupal. To ensure the security of your server, you should update immediately! See the available updates page for more information and to install your missing updates."
Screenshots attached for reference.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 70: change-words-so-2545520-70.patch, failed testing.

mgifford’s picture

Assigned: tameeshb » Unassigned

@tameeshb please re-assign if you have time to work on a new patch.

tar_inet’s picture

In opposition to #77 I still can see the issue on 8.4.x branch

So, based on #70 I´ve created a new patch, hopefully it won´t fail applying.

tar_inet’s picture

Status: Needs work » Needs review
valthebald’s picture

Status: Needs review » Reviewed & tested by the community

I have applied the patch from #80 and confirm that it removes confusing link from the first sentence in status reports (screenshots from #80 apply)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

This is essentially the same patch as #70 so it has the same problems already mentioned above.

Also, I'm pretty sure Drupal 8 got rid of array() syntax in the interim, so it shouldn't be re-introduced.

ashishdalvi’s picture

Issue tags: +dcm2017

We will work on issue at DCM2017

benjifisher’s picture

Issue tags: +Baltimore2017

It will take some time to go through the comments, but I think this still counts as a novice task. It might be good for a couple of people to look at together.

Please pay attention to comments from @David_Rothstein.

As a first step, please update the issue summary. The "Proposed Resolution" section should clearly show the existing and proposed markup. At least, then people will know where to look instead of having to hunt though the comments to figure it out. We can always update the summary again if needed.

Amarshall’s picture

I am looking at this issue at Baltimore DrupalCon with Jlint in Room 301.

micheljp’s picture

Working on a new patch.

a11y.matters’s picture

Hey, we've (a11y.matters and edorsini) updated this issue during the DrupalCon Sprint Room 301. First patch attempt!

Moved the link location in the second sentence to the text "missing updates".

See the available updates page for more information and to install your missing updates.

It seems to be more logical to place the hyperlink for /updates/update on the "missing updates" text. It seems to remove the confusion.

a11y.matters’s picture

Status: Needs work » Needs review
mradcliffe’s picture

I mentioned this to @a11y.matters, @edorsini, and @micheljp that the issue summary needs to be updated per @benjifisher in comment #87.

lukas.fischer’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the patch #88. All fine. The solution is elegant as it does not change the text itself, just the link is placed at a much more meaningful place. This is a quick win, so it should be make it into core right away.

lukas.fischer’s picture

Issue summary: View changes
mradcliffe’s picture

Status: Reviewed & tested by the community » Needs work

1. The screenshots added in #88 should be added or replace the images of the screenshots in the issue summary.
2. I'm not sure if the proposed resolution changes match the expected format of what @benjifisher mentioned in #85. Maybe add the HTML markup changes as well (not the PHP) wrapped in code tags in the issue summary.
3. We should explain why we adding the link to "missing updates" is the more elegant approach. I am not an expert in language so that would be very useful for me.

lukas.fischer’s picture

Status: Needs work » Reviewed & tested by the community

@mradcliffe, ok I'll update the issue summary now.

lukas.fischer’s picture

Issue summary: View changes
lukas.fischer’s picture

Issue summary: View changes
lukas.fischer’s picture

Issue summary: View changes
lukas.fischer’s picture

I updated the issue summary to make it much more clear what this issue will fix.

lukas.fischer’s picture

Issue summary: View changes
catch’s picture

Assigned: Unassigned » yoroy
Issue tags: +Needs usability review

Going to ask yoroy to look at the text change here. I wonder whether we should say 'pending updates' instead of 'missing updates'?

rovo’s picture

Referring to 'available updates' twice, but arriving at two different pages still seems slightly confusing to me. Mentioned in #72, "it would be nice to improve the wording", I've made a minor attempt at that.

Wording after patch in #88
After Patch
<div class="description">There was a problem checking <a href="/admin/reports/updates">available updates</a> for Drupal. See the available updates page for more information and to install your <a href="/admin/reports/updates/update">missing updates</a>.</div>

Proposed Wording
Proposed Wording
<div class="description">There was a problem checking <a href="/admin/reports/updates">available updates</a> for Drupal. See the update page for more information and to install <a href="/admin/reports/updates/update">manual updates</a>.</div>

rovo’s picture

FileSize
29.55 KB
mradcliffe’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.06 KB

One thing that helps to review the patch is to provide an interdiff.

I have attached an interdiff between 88 to 94 to make it easier to see the changes. This should have also been done for the patch in #88, but I forgot to mention it.

I think the issue is going to be Needs Review until yoroy's review.

mradcliffe’s picture

Issue summary: View changes
Issue tags: -Novice

Removing Novice tag for now as there isn't anything to do at the moment.

rovo’s picture

FileSize
45.75 KB
52.74 KB

Thanks for the interdiff mradcliffe.

For yoroy's review, a couple other reasons for the suggested word change:
There was a problem checking available updates for Drupal. See the update page for more information and to install manual updates.

  • When you click the link 'available updates', the end-user is taken to a page titled Available Updates.
    Available Updates
  • When you click the link 'manual updates', the end-user is taken to a page titled Update.
    Manual Update Page
yoroy’s picture

Thanks for working on this. Two different destinations for identically worded links is indeed not good.

"Pending" is better than "Missing": with non-security related updates you might not be missing them, you just have no real reason yet to update. I think we can remove some friction by not explicitly calling out the names of the destination pages.

What about:

There was a problem checking for [available updates]. Review and [install pending updates].

I was going to suggest *Known* pending updates are listed on the updates page. but this sentence is shown without the preceding one as well and then it makes less sense to make "known" explicit. You link to another page because indeed there's more relevant information to be found there, probably not necessary to explicitly state that.

yoroy’s picture

Assigned: yoroy » Unassigned
NikitaJain’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
113.82 KB
131.71 KB
85.11 KB

Verified and tested on 8.4.x-dev, checked with the latest patch change-links-2545520-94.patch in #101 comment.
Here are the test observations:
Before applying the patch:
- The message shows for available updates on /admin/reports/status
"There is a security update available for your version of Drupal. To ensure the security of your server, you should update immediately! See the available updates page for more information and to install missing updates."
- The hyperlink present only on "Available updates"

After applying patch in #101:
- The message appears as:
"There is a security update available for your version of Drupal. To ensure the security of your server, you should update immediately! See the update page for more information and to install manual updates."
- The hyperlink present only on "Manual updates"
- When user click the link 'manual updates', the end-user is taken to a page titled Update.

Screenshots attached

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

@NikitaJain thanks for testing and the screenshots. I would like some feedback on my comments in #107 first though, so setting back to needs review for that.

lukas.fischer’s picture

@Yoroy, I agree to your suggestion - it's short, actionable and clear. I'm gonna create a patch with this wording.

"There was a problem checking for [available updates]. Review and [install pending updates]."

Status: Needs review » Needs work

The last submitted patch, 111: change_words_so_that-2545520-110.patch, failed testing.

benjifisher’s picture

Status: Needs work » Needs review

It looks like a testbot failure, now resolved. Back to NR.

lukas.fischer’s picture

Status: Needs review » Reviewed & tested by the community

There was a problem with another part of Drupal with "PHP 5.6 & MySQL 5.5 21,922 pass" which was not related to the patch #111. I retested and now it works - all green.

mradcliffe’s picture

Issue summary: View changes

I updated the issue summary so the attached images in #110 appear there instead of older screenshots, and updated the resolution to reflect the latest approved changes.

Also, since these are string/localization changes, we should not back port to 8.3.x. Translators on localize.drupal.org will need to update their translations for 8.4.x.

mradcliffe’s picture

Issue summary: View changes

Fixed link urls for images. Whoops.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
-        $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [':update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
+        $text = t('There was a problem checking <a href=":update-report">available updates</a>.', [':update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
       }
       else {
         $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [':update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);

Why is the first sentence being shortened but not the second? That makes things inconsistent.

-      $requirement['description'][] = ['#prefix' => ' ', '#markup' => t('See the <a href=":available_updates">available updates</a> page for more information and to install your missing updates.', [':available_updates' => \Drupal::url('update.report_update')])];
+      $requirement['description'][] = ['#prefix' => ' ', '#markup' => t('Review and <a href=":available_updates">install pending updates</a>.', [':available_updates' => \Drupal::url('update.report_update')])];

By wordsmithing this, you wind up with a fix that is harder to backport (due to broken translations, etc).

Instead of wordsmithing it, why not just remove it (as previously discussed above)?

Again, (1) the text is simply wrong under the conditions being discussed in this issue, (2) it's equally wrong before and after the above text changes (there are no missing/pending updates of the given type to install), and (3) it was never supposed to display in this situation in the first place (doing so was an accidental change introduced in Drupal 7).

yoroy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Apologies, we should definitely go for the simplest possible fix here. I was going from what's outlined in the issue summary, which does not mention what's *wrong* with the current text. Looks like the issue summary needs an update then.

vulcanr’s picture

#118 +1, I agree. Every user has a different approach for this and a different understanding of this.

Gábor Hojtsy’s picture

Reviewing in the UX meeting, we did not have time to read the 119 preceding comments, sorry. So this may be repeating points.

  1. +++ b/core/modules/update/update.install
    @@ -113,7 +113,7 @@ function _update_requirement_check($project, $type) {
    +      $requirement['description'][] = ['#prefix' => ' ', '#markup' => t('Review and <a href=":available_updates">install pending updates</a>.', [':available_updates' => \Drupal::url('update.report_update')])];
    

    I don't think you can *install* the updates, so saying in the link you can is definitely misleading.

  2. +++ b/core/modules/update/update.install
    @@ -113,7 +113,7 @@ function _update_requirement_check($project, $type) {
           $requirement['description'][] = ['#prefix' => ' ', '#markup' => t('See the <a href=":available_updates">available updates</a> page for more information.', [':available_updates' => \Drupal::url('update.status')])];
    

    Would this need to be updated as well?

  3. +++ b/core/modules/update/update.module
    @@ -550,7 +550,7 @@ function _update_message_text($msg_type, $msg_reason, $langcode = NULL) {
    -        $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [':update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
    +        $text = t('There was a problem checking <a href=":update-report">available updates</a>.', [':update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
    

    So "Drupal" clarified what type of update check was a problem (see the module and theme case below). By removing this part, the sentence is simpler, but the clarification is missing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

David_Rothstein’s picture

To hopefully wrap this issue up, here's a patch that goes back to the earlier-discussed approach of just removing the second sentence (the one that contains the duplicate link) when it serves no purpose being there.

There are a number of possible scenarios where the duplicate links were appearing, so here are screenshots of how all of them behave with the new patch:

  1. UPDATE_UNKNOWN error
    (occurs during scenarios such as lack of data for this release from update.drupal.org)

    The Status Report now looks like this:

    The "available updates" link goes to the Available Updates page, which makes sense:

    The link that was removed went to the Update Manager page, which is not useful here anyway:

  2. UPDATE_NOT_FETCHED error
    (occurs during scenarios such as a failure to contact update.drupal.org)

    The Status Report now looks like this:

    The "available updates" link goes to the Available Updates page, which makes sense:

    The link that was removed went to the Update Manager page, which is not useful here anyway:

  3. UPDATE_NOT_CHECKED error
    (occurs in the case of a dev release with no datestamp; in real life, this is mostly encountered when using a Git checkout of Drupal core; I am not sure if it's even realistically possible to see it with a contrib module but I faked it for the screenshots below)

    The Status Report now looks like this:

    The "available updates" link goes to the Available Updates page, which makes sense and provides additional clarification:

    The link that was removed went to the Update Manager page, which in this case does offer an available "update" (so in theory could be useful), but it's very likely to be a downgrade in practice, so I don't think directing them here was particularly good anyway:

  4. UPDATE_FETCH_PENDING error
    (occurs when the .info[.yml] file has been recently updated and update status consequently needs to be refreshed due to the new/updated version of the module)

    The Status Report now looks like this:

    In this case, the mere act of visiting the update pages will perform the necessary update status recheck. Thus when you click through the link to get there, the data will have been updated, and depending on what it finds, maybe there is or isn't an update that needs to be performed. The screnshots below are for a case where everything was, in fact, up to date.

    The "available updates" link in the above screenshot goes to the Available Updates page:

    The link that was removed went to the Update Manager page:

    Since we don't know what we're going to find until after the link is clicked, the behavior in this patch (going to the Available Updates page) is probably still best since it provides the most detail. (It might be nice if we simply ran the refresh on the Status Report page as well; therefore we'd never even see this status on that page. But that's a separate issue.)

The bottom line is that in all cases above, the new behavior should make more sense than the old behavior, while the duplicate links are also gone.

Meanwhile, in the case where there are actually updates to install, the patch changes nothing, and the second sentence is still appended, because that's the case where it actually does make sense and which it was designed for:

David_Rothstein’s picture

Title: Change words so that the same link "available updates" does not link to two different pages in the same paragraph » The same link "available updates" links to two different pages in the same paragraph when there is a problem checking available updates
Issue summary: View changes
FileSize
71.26 KB
32.14 KB
101.35 KB
54.26 KB
71.52 KB
32.35 KB
73.54 KB
32.36 KB

Updated the issue summary to reflect the above patch.

Note that in the patch:

@@ -136,6 +138,7 @@ function _update_requirement_check($project, $type) {
     case UPDATE_UNKNOWN:
     case UPDATE_NOT_CHECKED:
     case UPDATE_NOT_FETCHED:
+    case UPDATE_FETCH_PENDING:
       $requirement_label = isset($project['reason']) ? $project['reason'] : t('Can not determine status');
       $requirement['severity'] = REQUIREMENT_WARNING;
       break;

This part is sort of an independent bugfix, but it's necessary in order for the above screenshots in the UPDATE_FETCH_PENDING case to be at all sensible.

David_Rothstein’s picture

The patch in #122 is deliberately as simple as possible, and it does not break any translations. This means I left out a couple things discussed previously in this issue:

  1. I left out any changes to remove "for Drupal" or "for your modules or themes" from the "There was a problem checking available updates" sentence. I think that's still worth considering, and I don't agree with this comment from #120:

    So "Drupal" clarified what type of update check was a problem (see the module and theme case below). By removing this part, the sentence is simpler, but the clarification is missing

    because as can be seen from the above screenshots, that clarification is already provided in the adjacent table columns.

    But, that change breaks translations and is not strictly related to this issue, so it's better to have a separate issue for it afterwards.

  2. Replacement of "See the available updates page for more information and to install your missing updates" with "Review and install pending updates" (as proposed in #106). I think that's definitely worth considering as improved wording in the case where there are actually are pending updates such that this sentence needs to be displayed - but again, that should be a separate issue.
David_Rothstein’s picture

Issue summary: View changes
benjifisher’s picture

@yoroy added the "Needs issue summary update" tag in #118:

I was going from what's outlined in the issue summary, which does not mention what's *wrong* with the current text.

@David_Rothstein updated the summary (and the title) in #123, with a further tweak in #125. The summary now explains the problem, so I think we can remove the tag.

I am adding the "Needs followup" tag based on the two comments in #124.

To help with the review and to make it clear that this patch really is "as simple as possible", I have attached a do-not-test patch:

  1. I rewrapped some comment lines to make it clear what had been added.
  2. I ignored indentation changes, creating the patch with git diff -w -U4 8.5.x

IMO the patch (from #122) is RTBC, but the issue summary says "Waiting on usability review from yoroy", so I will hold off.

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

I created a single follow-up issue for the points mentioned in #124: #2919559: Simplify text describing updates in the site status report. I am removing the tag "Needs followup".

I am marking the patch from #122 RTBC despite the request for a review from @yoroy, who has said that he does not want to be a bottleneck for all Usability issues.

Again, the patch I attached in #126 is only to help with review. It should not be committed.

The last submitted patch, 122: update-status-2545520-122.patch, failed testing. View results

benjifisher’s picture

xjm’s picture

We committers can use a word diff or whitespace diff with the best of them, so we don't really need a -do-not-test patch for a five-line change. What will actually help us the most is ensuring that the correct patch to apply is attached as the final patch on the issue and displayed correctly in the issue summary. This is especially important in such a long issue where there's a chance of reviewing the wrong patch othewise. RTBC issues also automatically retest that patch, so if it's not there the correct patch will not be retested.

I'm reuploading @David_Rothstein's patch from #122 to address this.

Thanks!

xjm credited edorsini.

xjm’s picture

Saving issue credit and adding a contributor who collaborated on an earlier patch with another sprinter.

xjm’s picture

Version: 8.5.x-dev » 8.4.x-dev

Alright, I read over #122 and I agree that this change makes sense in all scenarios. We're now adding one message in all scenarios except those covered by those four update statuses, and then a different message in those four scenarios.

Committed to 8.5.x. Since this doesn't actually change strings, but instead just changes the situations under which existing strings are displayed, it's safe to backport, and I think we should do so since it's also an accessibility bugfix. We're in commit freeze right now for 8.4.x for the patch release tomorrow, so leaving RTBC against 8.4.x.

Once this is cherry-picked to 8.4.x we can move it to "Patch (to be ported)" for the D7 issue.

Thanks to everyone who's helped review and test this. I think the contributor list is longer than the patch. :)

  • xjm committed 8d2a2ec on 8.5.x
    Issue #2545520 by sdstyles, BarisW, David_Rothstein, owenpm3, mgifford,...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 130: update-status-2545520-122.patch, failed testing. View results

benjifisher’s picture

Status: Needs work » Reviewed & tested by the community

The patch failed to apply to 8.5.x, since it has already been committed to that branch. I am not sure why the testbot tried that, since the issue is now set to 8.4.x, but let's try setting it back to RTBC.

  • xjm committed adb75be on 8.4.x
    Issue #2545520 by sdstyles, BarisW, David_Rothstein, owenpm3, mgifford,...
xjm’s picture

Version: 8.4.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

And now cherry-picked to 8.4.x as well. Thanks everyone for your tireless review and testing on this issue!

Setting "Patch (to be ported)" for D7. A separate child issue should be created for the change in D7, and then this can be marked back to fixed in 8.4.x once the issue is referenced.

benjifisher’s picture

@xjm, thanks for the instructions. I did not know that the policy was to create a child issue for the D7 port.

I created the child issue #2920487: The same link "available updates" links to two different pages in the same paragraph when there is a problem checking available updates. Is it a problem that the two issues have the same title? I am not sure if someone else should review that or if I should change the version and status of this issue myself. I guess I will do that in a few days unless someone else does it first.

benjifisher’s picture

Status: Patch (to be ported) » Fixed
benjifisher’s picture

Version: 7.x-dev » 8.4.x-dev

Oops, forgot to set the version back when changing from "Patch to be ported" to "Fixed".

Status: Fixed » Closed (fixed)

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