Move developer info into a block provided by cvs.module

The patch attached does the same thing as the one provided in #372580: Move the project node links to blocks and adds the "Developers for project" block.

Here is a new patch. I fixed some errors (like sorting), and took advantage of this patch : #373296: Expose a method to get a project's contributors array.

I added the possibility to limit the number of developers displayed in the block, and to add a "More developers" link.

New version of the patch.

• defines "Project: Links" block
• defines "Project: Get involved" block
• defines "Project: Developpers" block:
  • displays all commiters on the project
  • has configuration: number of commiters displayed, more link
  • needs #373296: Expose a method to get a project's contributors array

A) It's dumb that the code for displaying CVS stuff lives in project.module. This should move to cvs.module.

B) We want this info as a block (which lists the top-5 by default, and has a little JS to magically expand itself to see the full list).

I'm going to work on this now.

At #431388-8: Rename core's project node to "Drupal Core" dww mentioned that this block at http://infrastructure.drupal.org/drupal.org-style-guide/prototype/core.html would be limited to the top N maintainers. Could we please base it off of activity date instead?

Also, would it not be better to views-enable this info and handle it there rather than in a hard-coded custom block?

@webchick: Thanks for the feedback...

- we don't want a giant block with 12 names if 12 people happen to have committed in the last year. it's already by date -- it's ordered by date. it's just a question of how long we let the list grow before we hit the "read more" link. ;) Are you saying you want both a max # and date filter, so you only ever get the N most recent committers that have committed within the last M months or something? Seems overly complicated. We already print the dates of the last commit and first commit in the block, so if you want to see that the most recent commit is from 9 months ago, you can see that directly, instead of having the block disappear with no commits in the last 6 months...

- It's not really so easy to views-ify this info, since we're doing COUNT(), MIN(), MAX(), etc. It's not as simple as exposing some fields from a table. I think views *can* handle computed fields like this, but it's really vastly easier to just have our own block for this (at least for now). Plus, I'd rather not sink time into views-ifying cvs.module -- I'd rather be doing that to versioncontrol_cvs...

- we don't want a giant block with 12 names if 12 people happen to have committed in the last year.

I was thinking more "who has committed in the last 60 days" and not that the block would go away but that it would simply indicate that there's been no activity, click here to see all the maintainers for the project, which is the full list sorted by last activity. Would:

a) Make it easy to tell at a glance that no one is currently maintaining a given module, which would also make it easier for those who want to take it over to justify that there's no activity
b) Provide incentive for developers to keep active so their name shows up in big letters on the main project page.

Although I suppose you could argue that commits aren't everything, but it's very rare to find a module so bug-free that it doesn't need attention every few weeks if it's being well-maintained. Something like CVS Deploy and Update Notifications Disable being the obvious exceptions to the rule, I suppose. ;)

It just is a bit confusing because the point of the block seems to be to highlight active maintainers, and it doesn't really make sense for someone like Steven (or JonBob in the case of CCK) to be on the block, even though he would be if we were to limit it to, for example, 5 people.

Anyway, I'm not hard-pressed one way or the other, so do what you'd like. :)

Well, the block filters by people who currently have CVS access. ;) So, if the concern is not having old maintainers still showing up, that's already taken care of. That also solves one of my other major concerns: I *really* don't want translators to show up in that block just because they commit .po files. But, since they don't have explicit CVS access, they don't count as far as that block is concerned. ;)

As I see it, the point of the block is "who's supposed to be maintaining this", not "who's committed in the last 60 days". If they're not committing, their "last commit" time gets bigger and bigger. I'd rather see that immediately than have all the names disappear and have to click to another page to see what's going on.

Ok fair enough. :)

Patches for project.module, cvs.module, bluebeach's style.css, and bluecheese's styles.css attached. Tested locally, working great. Maybe I'll try deploying these on d6.d.o so folks can see how they look there. That said, any objections or suggestions?

Core's block cache is actually going to be pretty useless for this, especially on d.o. It's cleared on every comment or node added on the site, so we're going to churn through cached blocks all the time, effectively having no caching at all.

Therefore, here's a new patch for cvs.module that adds a {cvs_cache_block} table to do custom caching of this block. We only invalidate the cache when a commit is made for a given project. Tested locally and it's working great.

Any final objections before I commit/deploy all this?

Thanks,
-Derek

This is now deployed on d6.d.o for testing:

http://d6.drupal.org/project/drupal
http://d6.drupal.org/project/project
http://d6.drupal.org/project/views
...

What do y'all think?

Based on some feedback in IRC, I updated the styling a bit on this (shift-reload those links from the previous comment). merlinofchaos pointed out that as long as this is still bluebeach, we should really keep the bullets on the li for this, for consistency with the rest of the blocks on the site. I also added some magic so that the "N commits" never wraps separately so it doesn't look so weird if the block is way too narrow (and/or the username(s) are way too long). I'm not convinced the bullets on the li look good, but I can see the argument about consistency with the other (ugly) blocks in bluebeach...

Any final suggestions or concerns before I commit this and deploy it on d.o?

couple of minor things is all i see:

1. function theme_cvs_project_maintainer_list($node, $maintainers, $max_maintainers = NULL) -- it looks like there's no way for $max_maintainers to ever be NULL, not via the UI at least. maybe we should add an 'all' option to the select list of how many maintainers to display, and check for that instead?
2. i don't see where $maintainer_join is ever set in function cvs_get_project_committers()

i'm wondering why we're not using the standard drupal cache API for our block caching here? i believe cache get/set/clear_all would be fine here, i think we would just to adjust {cvs_cache_block}'s schema to match the standard caching schema.

i would prefer to do it this way unless there is a compelling reason to do it otherwise. standardization == good.

@hunmonk: Thanks for the review.

Re: #15.1: Yeah, I guess we could do that.

Re: #15.2: Yup, that's a bug -- a hold-over from a previous implementation of this block. Now, that's handled via array_intersect_key($committers, cvs_get_project_maintainers($nid)); -- I realized it'd be a lot harder to actually do this JOIN correctly via SQL because of the way the {cvs_project_maintainers} table works: there's never a record in there for the project owner, only the *additional* maintainers. There are weird special-cases for that in a few places, we should probably change it at some point, but not for this. ;) Anyway, yes, that $maintainer_join should be removed.

Re #16: See #425728-21: Add an "issue cockpit" block for project nodes -- same question gets the same answer. ;)

Re: #16 -- In principle, I changed my mind: see #425728-22: Add an "issue cockpit" block for project nodes. In practice, we're sort of screwed to use the core cache API for this, anyway. ;) The best (only?) place to invalidate the cache is inside xcvs-loginfo.php. However, until #136866: Bootstrap Drupal so xcvs scripts can use Drupal database functions is done, that script is not bootstrapping drupal, and is just doing direct DB manipulations. So, we can't call cache_clear_all() inside xcvs-loginfo.php. :(

#136866 is too big a change to a critical part of the d.o infra just for invalidating this cache. For now, I'm going to stick with the separate table. If/when we fix #136866 or migrate to versioncontrol_cvs (which I believe is already bootstrapping Drupal for the integration scripts), we can revisit this and convert it to use cache_(set|get|clear_all) and the {cache} table...

That said, I'm going to reroll for #15 and get that finished up. Stay tuned.

This fixes both points in #15, and adds code to invalidate the cache when you save the configuration of the block (e.g. to change the length of the list).

i agree that we shouldn't be messing w/ adding any bootstrapping stuff to the xcvs scripts at this point. code changes from the feedback in #15 look good, so if it's been tested i say we're good.

Committed #19 to HEAD of cvslog, #11 to HEAD of project, and some initial CSS to bluebeach and bluecheese in SVN.

Now this just needs to be deployed and configured. ;)

Now deployed on d.o.

FYI: There was a minor bug with the caching I just fixed over at #457878: 'Maintainers for <project>' block shows outdated informations...

Now also ported to in Version Control API, with a few minor adaptions to prepare for different version control systems. You'll hardly notice as long as you stick with the CVS backend :P