The code that creates the list of users who are maintainers of a project (the CVS access tab from the project's summary page) does not check to make sure that all users who are cvs maintainers of a project have the cvs_accounts status as CVS_APPROVED. Though rare, this could happen if a user with a valid cvs account is added as a maintainer for a project and subsequently has his cvs account status changed to something else, for example disabled.

This patch will work on it's own, but for the user interface to match what is actually happening, the patch at http://drupal.org/node/154280 also needs to be applied.

AC

CommentFileSizeAuthor
#1 cvs-access.png4 KBaclight
cvslog_project_cvs_access.txt2.55 KBaclight

Comments

aclight’s picture

aclight commented
StatusFileSize
new cvs-access.png4 KB

Here is a screenshot with this patch applied

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

I committed http://drupal.org/node/154280, so I took a look at this. Once again, whitespace and comments were broken in here. In the future, please be more careful about this, since it just creates more work for me. A visual inspection of your patches before you post them should immediately alert you to trouble, if lines you didn't really need to touch for the change end up showing up in your diff, it means you tweaked the whitespace in some bad way...

Anyway, back to the patch... ;)

A) Nice work on ripping out the hard-coded 'contributions' part, that was a hack. However, this is bogus:
'%cvs_dir' => '' . $node->cvs_directory. If you're going to rip it out, don't leave the empty string in there. ;)

B) I know it was my fault originally, but the whitespace for the string concatenations was wrong around here:

$output = '<p>' . t(...

Should be:

$output = '<p>'. t(...

(no space between the string literal and the .) You got this right later in the patch, so you seem to be aware of this convention. In general, I despise patches that separately fix code style among a bunch of other changes, but if you're already touching a line, you might as well make your new version of the same line comply with the code style. ;) Soon I'll be doing a thorough code style fix on all of project* in all branches, just to make sure we're always starting from a clean slate...

C) This makes me a little nervous:

+      $username = '<del>'. theme('username', $user) .'</del> <em>('. t('CVS access disabled') .')</em>';

Almost seems like a candidate for a theme function...

Anyway, I fixed A, B, and the other whitespace/comment issues. I'm chosing to ignore C (someone can convert it into a theme function later if they really feel strongly). ;)

Tested (works nicely) and committed to HEAD and DRUPAL-4-7--2.

Thanks,
-Derek

aclight’s picture

aclight commented

Once again, whitespace and comments were broken in here. In the future, please be more careful about this, since it just creates more work for me. A visual inspection of your patches before you post them should immediately alert you to trouble, if lines you didn't really need to touch for the change end up showing up in your diff, it means you tweaked the whitespace in some bad way...

I only see 2 lines where whitespace might have been an issue:

-  project_project_set_breadcrumb($node, TRUE);
-
-  $output = '<p>' . t("This page controls CVS access for the %title project. ......
+  project_project_set_breadcrumb($node, TRUE); 
+    
+  $output = '<p>' . t("This page controls CVS access for the %title project. ........

What comments were a problem?

C) This makes me a little nervous:
+ $username = ''. theme('username', $user) .' ('. t('CVS access disabled') .')';

Almost seems like a candidate for a theme function...

Yeah, I thought it might :) I looked at the way locale.module does this (when you search for a string, it shows you a table with the search results, and for some strings there is strikethrough for some of the text). It adds a class to the text and then takes care of the actual style change in a .css file. The CVS module doesn't have a style sheet, and I wasn't sure it was worth creating one just for this. We could probably put the style in the project.css style sheet, since project module is required by cvslog, but I wasn't sure if that would fly either.

AC

dww’s picture

dww
we/he/they
commented

Yeah, sorry it was late, didn't mean a harsh tone in the reply. You identified the whitespace problem, that's all it was (but still makes extra work for me).

Re: comments, the new style (which project* certainly isn't consistent about, yet) is complete sentences, with Capitalization and a period.

Re: theme function. Bah. ;) This will all be getting re-written and re-organized in the near future with jpetso's versioncontrol.module and related changes, so I'm not going to worry about it.

Thanks,
-Derek

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)