Make it easier to visually distinguish security updates on Updates report

Peter, can we get a "before" screenshot so people understand what is being fixed here? We could also use a summary of what you're trying to accomplish here.

Cool. This makes a heck of a lot of logical sense to me. It would be nice to get the thoughts of the update module maintainers. Changing component accordingly.

+      case UPDATE_NOT_CHECKED:
+      case UPDATE_UNKNOWN:
+        $class = 'unknown';
+        $icon = theme('image', 'misc/watchdog-warning.png', t('warning'), t('warning'));
+        break;

I'm not sure I agree with that though. If the update hasn't been checked, don't you not have cron running or something? Isn't that an actual "warning" rather than a mere "unknown" warning? We should probably remove UPDATE_NOT_CHECKED here and have it inherit the default warning behaviour?

Just nothing that this is a good improvement, we received concencus on IRC that this would be the way to go - argument beign that priority should be identified by color, and a new update is not neccairy high priority unless its a security release.

I also like this, 'no update information available' in grey works for me as well - I think I've seen that due to packaging script/.info issues or similar which aren't really critical.

catch: what about just the normal background white?

I agree that having the security update coloring stand out would be a rather good cause here. Also agree that having a gray background would be a better fit then just clear white.

I support this change too (but haven't reviewed the patch yet).

I'm also in favor, but haven't done a thorough review. Just starting to catch up a bit on the (very sad) update.module issue queue... :(

Re: pwolanin's excellent question in the screenshot in #5: "which of these is important?" -- the one with *slightly* more bold, silly! ;)

Re: patch #8: a few quick thoughts:

A) What does the 'update_notification_threshold' setting even do any more after this patch? It's been so long since I looked closely at any of this code that I'm not sure what else it impacts beyond how things look on this report. It probably still impacts email notifications, and perhaps what things look like at the status report page. But, it'd be a good idea to grep for it and be sure.

B) Given the results of (A), it seems like the help text for that setting needs to be updated, too. Maybe the setting itself needs to be re-thought...

dww's right; we can indeed remove this variable and setting; afaict, the only places it impacts in the code are removed in this patch. The description of that field implies that it impacts the e-mail sending functionality, but I don't see how it does that on a quick grep.

This looks good.

I had the same thought as dww before having read his comment. Can't we remove the setting? It seems more logical to turn that setting into an e-mail only setting at this point, or to still try to remove it.

pwolanin, the help text mentions "see these error messages". I think it would be good to revise the help text to remove the non-email stuff, and maybe to rename "Notification threshold" to "E-mail notifications" or something. In short, I would make it more clear that this is about the e-mail notifications.

#23 looks pretty good to me. I need to run right now and don't have time for a more thorough review, but I'll try to put this through the paces this afternoon (time permitting) or sometime on Tuesday (more likely).

However, if this is a "bug report" and we're planning to backport it to D6 (as per #1) I don't know how to reconcile the desire to make these UI changes for clarity vs. the prospect of breaking the string freeze in the stable release. I'd *love* to break the string freeze for a few other edge-case error messages in D6, but I get the feeling that's not going to fly:
#208766: Latest release from maintainer's recommended branch called "Also available", not "Recommended"
#210144: If currently installed release is not found, update.module prints bogus information
#237608: "Recommended version" for unsupported branch

If 6.11 is going to be a slightly unorthodox core release for update.module (e.g. schema update from #220592: Core cache API breaks update.module: fetches data way too often, kills site performance, etc and possible new hook from #445748: Add hook_update_projects_alter()) I'd be really happy to break the string freeze and clean up these other UI problems, too. No idea if that's going to work. @Dries/@Gabor: thoughts?

Patch seems good, however, this line "Note that each module or theme is part of a "project", which may or may not have the same name, and might include multiple modules or themes within it" is a bit confusing and I don't really understand it. I don't find the need for it.

Can I have a before/after?

@Bojhan:

Before: comment #5
After: comment #1

Hint: the only two images in this issue. ;)

Everything looks good to me. I'd mark RTBC except dww wanted a more thorough look.

The interface is oke, apart from code review this issue is RTBC.

Note: I also wanted an answer from Dries/Gabor about "can we break translations with this patch when we backport it?".

Seems like the changes only affect some possibly not often used settings (ie. not critical parts of the update reporting interface), so a string change would not be a problem. If you'd need to modify the critical update reporting parts, then that would be a problem IMHO.

@Gabor: Cool, thanks. Yes, the only strings changed in #23 are the title and description of a setting on the (rarely used) update module settings tab. The string changes from the other issues would potentially effect the update report itself, but only in pretty rare edge cases. Should I put any energy into D6 versions of those, or are they doomed? ;)

I've carefully reviewed and tested #37 and it is RTBC. I'm also happy to report that there are no patch conflicts if you apply both this and #220592-33: Core cache API breaks update.module: fetches data way too often, kills site performance, etc. ;)

I've reviewed #23 as well, though I don't currently have a working D7 test environment. However, since it's nearly identical to the D6 patch, and since update.module has barely changed in D7, I see no reason not to commit both.

Has my vote. FWIW, I tested D7 patch and everything works as advertised, so yes, RTBC.

Looks good! Committed to HEAD. Marking back to 6.x for Gábor.

Great, committed, thanks!

We should probably backport this to D5 contrib, too.

#44 didn't apply to DRUPAL-5--2 (it was only a partial backport). Finished the job, tested, and committed the attached patch to update_status DRUPAL-5--2. Setting this issue back to the original values for posterity. ;)