GHOP #65: Add "available update" indicator to installed modules

Warning: this is actually more complicated than it sounds. :( The modules page operates on the granularity of modules. update_status or update.module in core operates on the granularity of projects. A given project can contain multiple modules. See http://groups.drupal.org/node/7329#comment-21637 for more about this.

Furthermore, there's *a lot* of information in the update status report page, and I think it'll make the admin/build/modules table *way* too busy if we try to cram all that stuff into it. The update_status UI is just barely not too busy already, and that's not including module descriptions, enabled/disabled checkboxes, other links, etc, etc.

Plus, there's going to be a bunch of duplicate information on the modules page when a given project containing multiple modules (think views bonus pack, or even just views for that matter), has a new release available.

So, I believe the first step here is some design work and mockups. We need a carefully considered plan on exactly what this should look like. Then we can worry about the implementation (which, given a clear plan, should be relatively easy).

Cheers,
-Derek

Claimed by kourge: http://code.google.com/p/google-highly-open-participation-drupal/issues/...

It's not my task, but I'm going to toss in my two cents:

Because the relationship between project to module is one to many (i.e. there will never be a module in multiple projects), all we need to do is determine what project each module is a part of, and then have the module inherit the project's status.

I think that the modules install page should defer to update status for full information on the updates. Something like "View update status" as a menu link, along with the color change (which is fairly simple). We should probably warn people that any info on this page is advisory, and they should check update status for full info.

For the future generations, there is a question of whether or not these bells or whistles belong on the module install page, or if a new module dashboard page should be created that lets people do everything. While my patch is very cool :-) it is slightly hackish, and if we end up adding everything and the kitchen sink to the modules page, that solution won't be sustainable. Just a thought.

This is an early mockup. A few noteworthy things:

• As of now, there's a message on the top of the page. (see topmessage.png)
• It doesn't mention which modules have updates. It just says at least one of them do.
• If the user scrolls down, they'd see the module(s) in question highlighted in bloody red, grabbing their attention. (see mockup1.png)

I'm all for adding anchors on the available updates page for each project's row. That sounds fine. No time now to ponder webchick's other suggestions, but I'll come back to this later...

webchick: Most update program applications list only the software that needs to be updated. I think "And perhaps the warning message has a bulleted list of the modules so you see right away which ones are out of date?" would make the most sense, but only if these links went straight to the update status page, where full information about downloads, etc could be found. Otherwise, it's a bit pointless to force the user to click twice to get to the meaty stuff.

@ezyang: totally out of scope for this issue, but yeah, it'd kinda be nice if the available updates report was sorted by status, and all the "up to date" stuff was pushed to the bottom (or hidden in a collapsed fieldset or something). maybe you want to file a new issue about that?

FYI: the warning on the modules page is generated by update.module.

Here are a few of my current thoughts.

• admin/reports/updates should have named anchors (a.k.a. <a name="blah">) for convenient linking anchorage.
• The message on the top in admin/build/modules should have a bullet list that links to the anchors on admin/reports/updates aforementioned.
• The highlighted module table rows in admin/build/modules should only serve as an additional visual alert besides the message on the top.

From this, the message on the top will be the only click, should the user be interested in updating something or finding out more. This would also eliminate the jump-around-and-click-twice-to-finally-update-something situation that chx described.

linking "update available" would also help visually distinguish that text from the description. I find the mockup hard to read where the text blends in with the description too easily. different positioning could further improve clarity.

the text should read "security update required!" when update.module knows it's in that state.

moving this to the right component. this patch will probably only need to touch update, not system.

thanks!
-derek

So the "Update Available" part would also, like the bullet points in the message, link to the anchors on admin/reports/updates. I'll start patching away!

I ran the colors of the warning message through the Colour Contrast Checker at http://snook.ca/technical/colour_contrast/colour.html. It said the color difference is sub par. A text color of #A30000 would pass accessibility guidelines.

That should be a problem of Garland, the default theme for Drupal. It may also be a problem of color.module, since it shifts colors according to your customizations.

This is really taking quite some time than I thought it would be. Here are a few problems I'm having (and solving):

1. The granularity issues that dww mentioned are, for the most part, solved by dpr()ing my way through the data structure and adding extra foreach loops.
2. It doesn't do anything to disabled modules because as far as I know, neither does update.module itself......right?
3. I frequently had to call update_calculate_project_data() more times than I like it to be. Supposedly it should cache data so it shouldn't be costly. I'll know that I'm doing the wrong thing or calling the wrong function if dww gets horrified by the end of this list item.
4. Perhaps this repetitive routine can be refactored out into a separate function. I need some comments on this.
5. I needed to write an extra theming function called theme_update_module_list() to spit out the bullet list of modules in the drupal_set_message().
6. To highlight a specific module row, I needed to sneak the update status into the data structure that's passed to theme_system_modules() by using hook_form_alter(). Then I needed to modify theme_system_modules() itself so that it actually does the highlighting.
7. This is my current progress. Below are the problems / issues that I have yet to solve.
8. I have to anchorify the stuff on admin/reports/updates.
9. Then I have to linkify the bullet list items I've already implemented.
10. After that, I need to add the "Update available" fragment that I did in the mockup. This'll involve more modification of theme_system_modules().
11. Then I need to linkify those fragments too.

I've got to admit that if I had zero coding experience with Drupal, this might take, at the very least, three times the time I currently would need. Fortunately, I do have some coding experience with Drupal. Thanks to its nice docs, I was able to complete my first Drupal module last year in just three days!

Alright! I've got my patch rolled up and ready for review! And with a juicy, real, screenshot too.
Yes, I've noticed that the "Update available" text is a little bit crowding out the description, but that's something that can be trivially fixed in Garland's CSS stylesheet.

Cool, this is starting to look great. A few problems on a quick read of the patch:

A) In modules/system/system.admin.inc: the class for the row never gets set to "description" for the non-update case.

B) It's a problem that update_help() is directly invoking update_get_available(TRUE) and comparing the data. We already do all that work in update_requirements(), which we just called a few lines earlier in update_help(). :( I guess fundamentally the problem here is http://drupal.org/node/200028 where we should be caching the results of the hugely expensive comparison operation, and then make it easier for different parts of the code to just retrieve the cached data structure of the results. However, that's probably outside the scope of the GHOP task, so we might have to go the route of a few other GHOP tasks I've helped review, where the GHOP task is actually deemed completed before the patch is fully RTBC into the upstream code. We can't commit this patch as is, but that's not really kourge's fault. ;)

C) The only reason you're re-computing the status like this is to add the list of projects that have available updates to the little bullet list of modules we're printing out. Another approach instead of (B) would be to just populate that list directly in update_requirements() and stash it in a nested array inside the requirements array. Then update_help() just has to glue that together and fix up the message it's displaying, instead of itself doing any comparison work. However, this is basically a hack, and there are other good reasons to fix #200028, so we should probably ignore this approach.

D) Similarly, it's a drag that update_version_alter() is itself computing all the comparison code, since hook_help() fires on the same page. Sounds like another job for #200028. ;) However, I'm not sure this approach makes the most sense, regardless of caching. system already has to be modified to know about this data and test for it when constructing the module rows. So, it seems a little silly to have that code directly in system, but to populate the data only via form_alter(). I think it'd be cleaner to just have system test for update.module (which it has to do already), and then populate the data itself, instead of relying on form_alter() to do it in a somewhat backdoor/hackish way. Not faulting your patch, I might have done the same thing on my first draft, but it seems like it'd be cleaner to just acknowledge that if we want this functionality in core, than system.module has to know a little bit about update.module and act accordingly. However, before you spend any time on this item, I'd wait for some second opinions, especially from the core committers. ;)

E) The theme functions you added need doxygen comments. As per http://drupal.org/node/196667 they should all mention "@ingroup themeable" as well.

F) It's not clear why you have this at the front of theme_update_module_list():

  if (!is_array($data)) {
    return $data;
  }

You only ever call that function with an array. I can't really think of any reasonable use case where you'd invoke it with a string, so it's probably best to just write it to expect an array and document it as such.

G) You patch introduces some stray whitespace in modules/update/update.report.inc

So, other than the evil caching/performance problems that update.module already has (which this patch is making even more obvious), this mostly looks fine, with only a few minor problems to clean up. I'd wait for further comments about the form_alter() approach, but perhaps what you have will be considered core-worthy.

Great work!
Thanks,
-Derek

p.s. I forgot to mention, the patch doesn't apply completely clean:

patching file modules/system/system.admin.inc
Hunk #1 succeeded at 1967 (offset 5 lines).
patching file modules/update/update.module
patching file modules/update/update.report.inc
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 38 with fuzz 1 (offset 2 lines).

Since this patch is based on d6beta4, I'll make another that is based on CVS HEAD so that the patch will be clean(er).

First, fantastic work. hugely appreciated by us end users that just want to get their site running.

Can I challenge a basic assumption here? When I run a Drupal site and see a red error message, I correct it immediately... becuase Drupal will not work.
I almost NEVER upgrade a module on a site that is working. Mostly becuase often a module upgrade breaks something... small, large whatever.

In this model (in this patch), however, every time I visit admin I upgrade the module-- it says "To ensure proper operation of your site, you should update as soon as possible" in a big red error. In reality, if I update the module, I am very likely to END the proper operation of my site.

Proposal:
1. Delete the sentence "To ensure proper operation of your site, you should update as soon as possible" until most module maintainers get the hang of the release system.
2. Change the color from red to an 'advisory' color. Whatever is standard in Drupal... perhaps yellow (or is that that highlight standard?)

My cvs diff kept on spitting out funny patches, which caused quite a lot of confusion between me and webchick. Essentially the patch is missing + signs at the front of lines every now and then. I've fixed this manually despite webchick's objections, but just in case the hand-fixed patch isn't clean as well, I've also attached the files that I modified, as webchick suggested.

This patch is against CVS HEAD.

I'm actually not so sure if the attached files (which I supposedly modified) are as up-to-date as the patch, but since I've got to run I'll just cross my fingers, assume they are, and give it a go.

Ok, since I am another GHOP student, this review is not official, and another review is needed, but here's my feedback:

Code style issues:

-In your patch, empty lines are indented. They should not be. For example:

<?php
function abc() {
  //Line starts two spaces indented.
  
  //Previous line should not have been indented because it's empty.
}
?>

This occurs several times within your patch.

Major issues:

On the admin/build/modules page, on a Drupal 6.x.dev version as of last night, I get the following errors:

Sometimes this:

# notice: Undefined property: stdClass::$status in example\modules\system\system.admin.inc on line 897.
# notice: Undefined index: garland in example\modules\system\system.admin.inc on line 897.

But always with this:

# notice: Undefined property: stdClass::$name in example\modules\system\system.admin.inc on line 1924.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.
# notice: Undefined index: project in example\modules\system\system.admin.inc on line 1972.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1969.
# notice: Undefined index: update_status in example\modules\system\system.admin.inc on line 1970.

Minor issues:

-The "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." message is not really necessary anymore on the admin/build/modules page.

-For some reason, Drupal was claiming I had a module installed under the "other" category, but couldn't seem to find the module (in fact, I don't have any modules that fall under the "other" category). See the attached screenshot.

Again, this review doesn't actually count for anything, but hopefully it'll give you some ideas on how to improve it while waiting for official review.

-cwgordon7

Gordon, aside from the coding style, the major issues you've mentioned were exactually (yes, I'm waiting that word I coined to get added into the Merriam-Webster dictionary) what I encountered and fixed when I switched my patching environment from beta4 to HEAD. You may not be using third.patch, because I've just checked third.patch (the last one) and it does contain the fixes to the issues.

The main thing that's causing these issues is because update information isn't available for the version "dev" (obviously), but my patch is falsely assuming it is, thus iterating over a property that doesn't exist on each module. As for the phantom "Other" package, it came from the mistake of assuming everything that has update information is a module. Garland is one good example of something that belongs to a package (the package "drupal", part of core), has update information (when not using dev / HEAD), but isn't a module (it's a theme).

Luckily I encountered both of these edge cases when I switched my patching environment over from beta4 to HEAD. This should be fixed in third.patch, but if it's not on your patched environment, I'm thinking either one of the following happened. One, my patch is still funny despite me manually fixing it (thus making webchick right about how hand-fixed patches are risky). Two, some new stuff / code occurred on HEAD and it broke my patch.

I'll do a CVS update in my environment to see if the latter is the case, while the former possible case can be checked by extracting differences.zip and overwriting the files in your environment, *then* have CVS update try to merge it with the latest revisions on HEAD. (I guess it'll work, since it worked on my machine.)

I've fixed another issue that Gordon mentioned regarding the update notification threshold. In other news, my cvs diff seemed to stop spitting out funny patches, so here is a fourth patch that has the aforementioned issue along with the coding style addressed.

I added the documentations for the theming functions I added. I'm getting proud of my cvs diff because it completely ceased to misbehave and is now doing exactually what it should. I really hope this patch gets into core because it's a big usability plus.

(Sorry for the delay in reviewing, I was away from a computer for the last few days).

This is definitely getting closer.

The main problem I see now is that if both core and a contrib are missing an update, the error message on the modules page is appearing twice (once for core, once for contrib), but both messages list the full list of all projects that are missing updates. :( See attached screenshot for an example (with a stale version of core and devel from contrib).

My only other concern at this point is that I'm still not visually sold on how the icon and update warning text looks smooshed into the module description. Again, see the screenshot. I'm not sure what to suggest as an alternative, but I wanted to raise it.

In other news, this patch is going to conflict with http://drupal.org/node/197186 :( It'd be really nice to get Gabor or Dries to comment here about if this has any chance of going into 6.x at this point. If so, we should probably focus efforts on getting this in ASAP, so that we can avoid too much patch re-rolling and conflicts over at #197186.

A few things I've noticed from the screenshot.

1. The "Update available" texts aren't links, which is odd. The patch should have made the texts into links, turning them blue. If I were to point my finger at something, then it would be system.admin.inc, because if you call theme_update_version_status() without the $option parameter which is an array that should contain the key 'url_fragment', then the text would not turn into a link.
2. You're using beta 2, which is odd as well. I understand this is exactly what might cause the message to appear twice, but using a patch that was first developed for beta 4 then for HEAD on beta 2 might cause problems, although I personally think the possibility is pretty low.
3. There I go contradicting myself right away. For the message appearing twice, I believe this behavior is present without my patch. See line 57 of update.module within the update_help() hook. It's foreach (array('core', 'contrib') as $report_type), with a drupal_set_message() within the loop. I haven't had the chance (yet) to test this myself, since I've never encountered both core and contrib being outdated. Maybe I should test this on beta 4, since RC1 is out already.

I'm not really using beta2, I just hacked system.module to lie about that for testing. I'm using the latest code from HEAD.

I don't know why the "update available" texts aren't links, I'll have to look into that.

In terms of the message showing up twice, it's really 2 messages, one for core, one for contrib. It works like that on the status report, too. The main problem with your patch is that both messages (with different texts) include the full list of projects that need an update. Hope that makes sense.

I've just tested this on a real version of beta 4 and it really did happen. I'll work on patching that. A static variable should be able to solve that problem.
As for the links not showing up, it seems like it's not happening how my side. I'll hack system.module and see if it happens.

I've noticed that when testing this on the actual beta 4, the texts do turn into links, but when I hacked system.module, they didn't, so it seems like the texts not turning into links is from messing with the VERSION constant (making it not match up with the .info files of the core modules).

Aside from that, I've changed the list to be only attached to the end of the contrib message. I've also unset the drupal project from the list.

Refer to the attached screenshot and patch. Also see this for a good visualization of the patch.

Cool, the messages are better now, and avoid the duplication, so that's nice.

However, this still needs work, since the "Update available" texts are only links when the .info files define the "project" attribute. If you checkout a site directly from CVS, instead of downloading tarballs, that doesn't happen, since the "project" attribute is inserted into the .info files automatically by the d.o packaging script. However, there's some code in update.module to handle this, by figuring out if it's core based on the "package" attribute, or by looking at the filesystem. See update_get_project_name() from update.compare.inc for more. You should make sure you use that same function to populate your $options array, instead of only relying on what's in the .info file.

Note: I'm going out of town tomorrow morning for a week vacation, so I won't really be able to keep reviewing this patch for a little while. :( I hope you can find other people to review and give you feedback on your efforts. Good luck!

-Derek

I've implemented a fix that would try to get the project name if it's not in the .info file using update_get_project_name() as you've said.

New patch attached. Whoa, that rhymed! Sort of. Also, a good visualization of the patch.

* The way lines are wrapped are not according to the coding standards. Can we put all the OR-clauses on the same line?

* To me it feels like the watchdog icons are not a good match -- it looks somewhat ugly. I can't really put my finger on this one. Does anyone else find these icons to be somewhat odd?

* IMO, the status messages are too verbose. Why are there two status messages? I wouldn't list the project names as part of the message. I'd try to make it more crisp and to the point.

To partly answer Dries's concerns:

- The line wrapping is probably just mimicking some of the really huge OR logic in update.module that puts the clauses on separate lines for better readability. No one seemed to care when update.module went into core, and I find it much more readable this way. Kourge was no doubt just copying the style of the surrounding code.

- The watchdog icons have been our standard "error", "warning", and "ok" icons for quite a few versions of core now, and those were re-used in update.module both for consistency and because it was easier.

- There are 2 status messages because that's how it works on the status report page and in update_requirements(). I don't remember exactly why we chose 2 separate messages instead of one dynamic one, but I believe early in the update_status 5.x-2.* development, Earl and others thought it'd be better to have a stale version of core more clearly labeled than a single "something is stale" message, to help the signal to noise ratio for knowing when core is out of date (which is almost always a bigger problem than most of contrib). kourge is just reusing the exact same message text from what shows on the status report page, he's just making it more visible by putting it as a drupal_set_message() on the modules page. Adding the list of project names in those messages was meant to be a way to put everything you needed to know crisp and to the point in one place, instead of having to scroll down the whole page looking for oddly-colored rows (see #5 above). Especially given that we've already hit the official string freeze, introducing a single new message here seems a little unfortunate. :(

So, our choices are:
A) Live with the two messages.
B) Break the string freeze.
C) Postpone this usability enhancement to 7.x.

No sense re-rolling this patch until we decide on which of these 3 options we're going for. Dries/Gabor, any chance for a pointer in the right direction? Thanks.

Yes,

* I was mimicking the existing style for OR clauses in update.module. I initially did not put line breaks, I did after I noticed that there were line breaks in very long if statements.

* Although the icon are primarily used by watchdog, the icons were also used in admin/reports/updates, which is the reason why I chose to reuse them.

* The two messages in-a-row phenomenon is existing functionality before my patch was introduced. The patch only tacks a list onto one of the messages, namely, the one for contrib.

dww was pretty quick, since I was in school, so this is probably lagging 4+ hours behind. I couldn't have put it better myself.

I really hope this gets into core, because the patch itself purely does not break the string freeze, and it's a UX big plus.

Looks like there's no hope of getting this into 6.0 now. Moving to the 7.x queue. Let's hope this gets cleaned up and committed early in the 7.x cycle.

Moving.

This is a feature request.

...should we close this as a duplicate of #1355442: Add "Update available" indicator to the modules list (part of #538904: D8UX: Redesign Modules Page)?? If so, anyone that thinks their comments here are still valid (this is a very old issue) and should be preserved, please copy-paste your comments and ideas there.

No, I think we should close that as a duplicate of this. I just commented there to that effect. Thanks for the pointer.

Tagging.

This would still be handy in D8. Not sure it would need an accessibility review at this point.

While I agree it would be a nice feature to have, I have my concerns about how much slower this page would get because of having to check for new versions, so I'm not 100% percent convinced.

In any case, the latest patch on this issue is from 9 years ago, so this isn't a reroll, start a new patch for 8.x if you want to push this forward.

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!