based on my experience with the form_alter() approach to allowing other certain people to be able to assign issues to each other (see http://drupal.org/node/4354 and http://drupal.org/node/67251), along with my discussions with halkeye about his http://drupal.org/project/subversion module (see http://groups.drupal.org/node/790), i'm thinking i implemented the "cvs access" tab all wrong [1]. ;) here's my new proposal:

  1. code in the project module should optionally produce a "file access" tab (there'd be a checkbox on the project edit form to enable this). the tab would generate/remove records in a {project_file_maintainers} table. this table would serve the purpose of the existing {cvs_project_maintainers} table.
  2. the cvs.module (to the extent it cares at all) should just use {project_file_maintainers}. in fact, it's really the xcvs-* scripts that are going to care, but that'll be a relatively easy change.
  3. there would be a 2nd tab, with the same UI as the "file access" tab, called "issue maintainers". this tab would automatically include all the users with file access as "locked" rows. everyone listed in either table would be in the "assignees" list for each other.

so, you'd end up with 3 tiers: project owner (full powers), file maintainer (revision control access, whatever that means for your site, etc), issue maintainers (additional rights in the issue queue).

for extra credit, i'd like to see all projects where i'm an issue maintainer in my http://drupal.org/project/user listing. ;)

in fact, i think this "issue maintainer" notion (on a per-project basis) is the right way to handle the per-status access control we're currently doing site-wide via permissions. i think it makes far more sense to let project maintainers define that stuff if they care, and select what status values the various tiers (plus the default "authenticated users" and "anonymous users" roles) are allowed to use. but, that's another story. ;)

i could maybe imagine still wanting a hook for who can be assigned tickets, but i kind of doubt it.

the key thing is making these two tables not specific to CVS, etc, but generic uid/nid tables that different modules can interpret however they please. you might argue that it'd be better to just use a single table and have a other field(s) which would be set to some well-defined int constant(s). i could make arguments for either approach, i think both have pros and cons. either way simplifies some code and complicates other code, and since we'd never store duplicate records, 2 tables wouldn't take up more space. i suppose there'd be fewer total DB queries with 1 table, which is probably the winning argument. in that case, we just call it {project_maintainers} and add either 1 additional int field with different constants, or separate bool fields (if DBs had bools, of course) for each kind of access a given user should be granted. separate fields is more flexible, but i'm not sure if we need that flexibility yet.

-derek

[1] dries, killes, and chx will notice that this new approach is pretty similar what i proposed to do originally when i first started the contrib cvs access stuff, but they rightfully steered me towards a more simple approach that would be easier to get going immediately. ;) oh well, here we are a few months later, and i think we're already starting to outgrow that solution. such is life...

CommentFileSizeAuthor
#63 69556-63.project_maintainers.png74.98 KBdww
#59 project-permissions-3.png37.61 KBsun
#58 69556-58.project_maintainers.png74.95 KBdww
#57 69556-57.project_maintainers.patch53.06 KBdww
#57 69556-57.project_maintainers.interdiff.txt7.32 KBdww
#53 69556-53.project_maintainers.patch53.19 KBdww
#53 69556-53.project-maintainers.interdiff.txt2.9 KBdww
#51 69556-project-maintainers-51.patch50 KBmikey_p
#48 project-permissions-2.png66.47 KBsun
#47 69556-46.project_maintainers.png62.83 KBdww
#46 69556-46.project_maintainers.patch45.53 KBdww
#46 69556-46.project_maintainers.interdiff.txt4.82 KBdww
#44 69556-44.project_maintainers.patch45.54 KBdww
#44 69556-44.project_maintainers.interdiff.txt4.43 KBdww
#43 69556-43.project_maintainers.patch45.62 KBdww
#43 69556-43.project_maintainers.interdiff.txt13.3 KBdww
#39 project-permissions.png23.45 KBsun
#38 69556-38.project_maintainers.patch45.96 KBdww
#38 69556-38.project_maintainers.interdiff.txt1.1 KBdww
#38 69556-38.project_maintainers.png69.71 KBdww
#37 69556-36.project_maintainers.patch45.95 KBdww
#37 69556-36.project_maintainers.interdiff.txt13.94 KBdww
#37 69556-36.project_maintainers.png69.77 KBdww
#35 69556-35.project_maintainers.permission-legend-L-fieldset-open.png69.13 KBdww
#35 69556-35.project_maintainers.permission-legend-L-fieldset-default.png39.9 KBdww
#35 69556-35.project_maintainers.permission-legend-L-fieldset.patch45.19 KBdww
#35 69556-35.project_maintainers.permission-legend-K-theme.png65.07 KBdww
#35 69556-35.project_maintainers.permission-legend-K-theme.patch45 KBdww
#34 69556-34.project_maintainers.png37.69 KBdww
#33 69556-33.project_maintainers.patch44.71 KBdww
#33 69556-33.project_maintainers.interdiff.txt3.4 KBdww
#31 69556-project-maintainers-combined-with-tests-31.patch42.36 KBmikey_p
#30 69556-30.project_maintainers.patch36.36 KBdww
#30 69556-30.project_maintainers.interdiff.txt5.87 KBdww
#29 69556-29.project_maintainers.patch34.3 KBdww
#29 69556-29.project_maintainers.interdiff.txt8.24 KBdww
#27 69556-project-maintainers-25-combined-with-tests.patch38.79 KBmikey_p
#26 69556-project-maintainers-#25-with-tests.patch38.79 KBmikey_p
#26 69556-project-maintainers-TESTS-ONLY-26.patch.txt5.02 KBmikey_p
#25 69556-25.project_maintainers.patch35.15 KBdww
#25 69556-25.project_maintainers.interdiff.txt3.03 KBdww
#24 69556-24.project_maintainers.patch34.52 KBdww
#24 69556-24.project_maintainers.interdiff.txt1.98 KBdww
#23 69556-23.project_maintainers.patch34.31 KBdww
#23 69556-23.project_maintainers.interdiff.txt3.04 KBdww
#21 69556-21.project_maintainers.patch32.4 KBdww
#21 69556-21.project_maintainers.interdiff.txt4.35 KBdww
#20 69556-20.project_maintainers.patch29.82 KBdww
#20 69556-20.project_maintainers.interdiff.txt9.12 KBdww
#18 69556-18.project_maintainers.patch28.83 KBdww
#18 69556-18.project_maintainers.interdiff.txt11.76 KBdww
#12 69556-12.project_maintainers.patch28.09 KBdww
#12 69556-12.project_maintainers.interdiff.txt11.32 KBdww
#10 69556-10.project_maintainers.patch16.17 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

halkeye’s picture

ack, just lost my entire comment.

what about the project_users table
projectID(nid/pid), uid, issue_access, file_access, ....

or a more normalized way...
pid, uid, role

project_user_roles
roleID, role

Example:
{project_users}
1, 0, 1

{project_user_roles}
1, 'issues'
1, 'code'

function project_get_roles($nid, $uid) {
// get all project_users_role for this project / uid combo
}
catch’s picture

Version: 4.7.x-1.x-dev » 5.x-1.x-dev

Bump, +1 etc.. I think this'd make a pretty good DROP task. Unless there's been a major change in opinions on this, I'm happy to write one up.

sun’s picture

Hm. Doesn't this remove the need for subscriptions to projects? (See Allow subscription to individual project issues and Select from various users for assigning issues)

Basically, the outlined architecture here makes completely sense. However, as always, things can be seen from different perspectives:

  1. Project/PI -> Subscriptions: Like described here, owner, CVS access, and issue maintainers are defined by project* modules. To display issues from all maintained projects of a user, Subscriptions/Views may get a list of projects from project* modules.
  2. Subscriptions -> Project/PI: Anyone can subscribe to projects and see corresponding issues in project/user. Project* modules allow project maintainers to select additional issue maintainers from the list of subscribers, and the project owner to grant/revoke CVS access to/from users of that list.

In general, the 2nd option sounds more dynamic (f.e. if I unsubscribe from a project, my issue maintainer and CVS access privileges may "go", too?) and may result in better usability (selecting from a list of users is easier than to remind and type a username; the list of users for assigning CVS access may be filtered by users that actually have a CVS account, too).

hunmonk’s picture

adding redesign tags

dww’s picture

Version: 5.x-1.x-dev » 6.x-1.x-dev
Assigned: Unassigned » dww
Issue tags: +git phase 2

The UI I proposed in the original post is wrong. Otherwise, this is an important plan, and will help with the Git migration since it's going to be the way we solve #781344: Extend project maintainer UI for manipulating git project ACLs. My basic idea is a simple "Maintainers" tab. It's a table with rows for each user. Project will provide its own checkboxes. Other modules will be able to extend this UI to add other checkboxes for other permissions each maintainer can access (maintain issues, releases, version control, etc).

I'm not sure if this extending will be via hook_form_alter() and each module is responsible for its own data, or if there should be a hook like hook_project_maintainer_acl_info() (and an alter hook) so modules can advertise what maintainer ACLs they want to add, and then project does all the form code and alters the schema on a {project_maintainer} table to add columns for the other perms.

dww’s picture

I'll be working on this as part of redesign sprint #2...

dww’s picture

After a few discussions with both hunmonk and mikey_p about this, we're all in agreement that each module should be responsible for its own data. The wonky hook I propose has its own problems, and will make the code more complicated. So long as I write the form and theme function for the form to be friendly towards altering, I think it'll be very easy to have project_issue, project_release, cvs.module, versioncontrol_project, etc, all form_alter() this, and each one will manage its own data. When the project is loaded, each module can query its own table and inject the right stuff into $node->project['maintainers'][$uid][$permission] as appropriate. That's the basic code plan.

In terms of the actual permissions, here's the current list (suggestions for improvement are always welcome):

Administer project
[project] Allows access to node/N/edit
Administer project maintainers
[project] Allows write access to node/N/maintainers
Maintain issues
[project_issues] Assign + be assigned issues to other issue maintainers. We might think of other things to use this perm for...
Maintain releases
[project_release] Access to node/add/project-release/N and to node/N/edit/releases
Write to CVS
[cvs] What the existing "CVS access tab" allows -- commits + tags, etc. We'll just continue to use the {cvs_project_maintainers} table -- nothing will change except the code around the UI will all disappear, and there will be a trivial form_alter() and custom submit handler to keep {cvs_project_maintainers} accurate.

Once all this is done, over at #781344: Extend project maintainer UI for manipulating git project ACLs we'll add a "Write to version control" checkbox to this UI for moving forward towards Git.

sun’s picture

Makes sense to me, didn't spot any issues.

dww’s picture

"Maintain releases" should be "Administer releases" given the allowed actions.

Maybe also"Write to CVS" becomes "Administer CVS" for consistency? All of those are severe powers you wouldn't want to give out lightly so they should probably all start with "administer". That would help contrast them with "maintain issues" which you can give to just about anyone.

Maybe "Maintain releases" would mean only editorial power on the release notes, e.g. for doc-friendly contributors who want to keep the notes clear and updated with known issues? KISS - unless we really need this, let's not add it yet.

dww’s picture

Status: Active » Needs review
FileSize
16.17 KB

Okay, here's a lightly-tested first draft. This doesn't touch cvs.module yet at all, but it provides the main UI in Project, fixes up some of Project's own internal access checking to use it, etc. I think it probably makes sense to handle project_release in this same issue, but to open new issues for project_issue and cvs.module to take advantage of this system and provide the other permissions I outlined in #7 and #9.

mikey_p’s picture

Planning to write some tests for this, so: subscribing ;)

dww’s picture

Here's a patch that adds an 'Administer project releases' checkbox via project_release. Mostly, this is a win. My form and theme function from #10 make it very easy to alter this form. One open questions about implementation details:

A) Does it make sense for project_maintainer_(add|update|remove)() to invoke a hook to give the sub-modules a chance to add/update/remove their own records, or should it all remain separate like I have it in this patch? I can clarify this question if it doesn't make sense on its own.

Technically, this needs work for (at least) a few other things:

B) There's no way in the UI to remove maintainers -- you can deselect all their perms, but you can't remove their row from the form. Not sure if a "Remove" column of checkboxes makes sense or if that'd be easy to confuse with permissions. Maybe a "remove" link in an "Operations" column would be more obvious (it'd bring you to a confirm form, like we have with the "delete" links on the CVS access tab).

C) We should probably special-case the project owner in the UI to always select all permissions and disable the checkboxes. You shouldn't be able to remove perms from the project owner herself. In fact, project_check_admin_access() already always grants access to the project owner, so the UI should agree.

See also:
#880818: Remove CVS access tab in favor of the project maintainers form
#880820: Add 'Maintain issues' checkbox to the project maintainers form

dww’s picture

Title: move "cvs access" tab into project, make it generic, and add issue maintainers » move "cvs access" tab into project, make it generic

Actually, "... and add issue maintainers" is now at #880820: Add 'Maintain issues' checkbox to the project maintainers form, so better title...

mikey_p’s picture

A few notes...

A) To my eye, a hook such as hook_project_maintainer_(add|update|remove) that is invoked from the submit handler for project_maintainers_form() makes more sense then a hook to call other hooks. I only say this seeing that project_maintainer_(add|update|remove) and project_release_project_maintainer_(add|update|remove) all have a similar pattern (and identical signatures between modules).I see no reason that this hook couldn't be invoked directly from project_maintainers_form_submit().

This would also remove the need for a submit handler in the secondary module, and make the form alter simpler as well ;)

B) I think a remove link with a confirm form is the most direct and clear way to handle removing users (and very Drupal-like ;).

C) In IRC we discussed making sure that there are records in the DB reflecting this even if no checkboxes are displayed in the UI (or they are disabled). If this is the way you go, this'll need to be included in the submit handler for the maintainers form so that each module gets the same data to insert via the hooks described in A.

D) After looking at A, why not take the same approach with the form items and permissions defined in hook_project_permission_info()? As it is, each sub module is invoking this hook on behalf of itself manually. These could be added by making project_maintainers_form() invoke hook_project_permissions_info() and building the form with that data. I believe with this and the changes mentioned in A that the form_alter could be removed from modules that are implemented project_maintainer permissions!

In summary with the above changes, a module would have to implement the hook_project_permissions_info, the hook_project_maintainer_(add|update|remove) hooks, with appropriate storage, and the loading mechanism (perhaps this could be moved to a standard hook as well?).

dww’s picture

@mikey_p: Excellent feedback. That's exactly what I was getting at in terms of the hooks. I'll double-check w/ hunmonk to make sure he's on the same page, then re-roll this with that approach. Hopefully should be a smaller and cleaner patch. Once that's happy, I'll move on to cvs.module and project_issue.module... Thanks for the review! Can't wait to see the tests for this. ;)

Cheers,
-Derek

mikey_p’s picture

Status: Needs review » Needs work

In testing I was running into the following notices on on the node/%project_node/maintainers tab:

Notice: Undefined offset: 0 in tablesort_get_order() (line 169 of /Users/mdp/www/project/includes/tablesort.inc).
Notice: Undefined offset: 0 in tablesort_get_order() (line 174 of /Users/mdp/www/project/includes/tablesort.inc).

I didn't get a change to look into this but I didn't see anything that should be triggering tablesort.

dww’s picture

That's funny, I didn't use tablesort at all on that tab. ;)

dww’s picture

Based on an IRC conversation with hunmonk and mikey_p about patch #12 and comment #15, here's a new patch. Definitely simpler. No form alter or submit handler needed at all in project_release. A few new open questions:

E) The way that hook_project_permission_info() works and how it interacts with t() is a bit weird. I'm trying to be t()-friendly, but it's a bit messy. We want the code to always use the lower-case English version for consistency (like core does), but we still want the table headers to be translatable at node/N/maintainers. What I'm doing in the patch is a bit of a hack -- I run the English version through drupal_ucfirst() and wrap that in t(). Then, to make t() happy, we define the t() versions with string literals in in implementation of the info hooks, so that those will be picked up when making a translation template.

A possibly cleaner alternative is to just have the info hook return nested arrays with a few different versions of the permission names. So, instead of this:

function project_project_permission_info() {
  // When these permissions are displayed in the UI, they're capitalized and    
  // run through t(). To ensure that the translation template knows about       
  // them, we define the t()'ed versions here. However, in the code itself, we  
  // always want to operate on the English version of the permissions, so we    
  // return those for the info hook itself.                                     
  /// @todo: This could move into a separate project.potx.inc file...           
  $translations = array(
    t('Administer projects'),
    t('Administer project maintainers'),
  );
  return array(
    'administer_project' => 'administer projects',
    'administer_project_maintainers' => 'administer project maintainers',
  );
}

We'd have this:

function project_project_permission_info() {
  return array(
    'administer_project' => array(
      'code' => 'administer projects',
      'ui' => t('Administer projects'),
    ),
    'administer_project_maintainers' => array(
      'code' => 'administer project maintainers',
      'ui' => t('Administer project maintainers'),
    ),
  );
}

Then, all the call sites would have to grab the version they need for whatever they're doing. Not sure it's a win or not. ;)

Doesn't D7 core have this problem already? I'll go look at what core does for this stuff now. Meanwhile, here's the new patch (and interdiff vs. #12) for review. Still needs work for (at least) #12.B, #12.C, and perhaps #16.

dww’s picture

Right, so D7 indeed returns a structured array for hook_permissions:

http://api.drupal.org/api/function/hook_permission/7

They just use the lower-case English version as the array keys, then 'title' for what I'm calling 'ui', and they allow for a 'description' field, too (although I'm not sure how we'd fit those into this UI -- maybe there'd be a separate table of just the permission names and their descriptions as a legend at the top of the page?). I've been using the foo_bar_baz key since we need to know the DB column names, too. Maybe instead of reporting that, we could just use the convention that the DB column is always the english version with s/ /_/. In fact, we mostly hard-code those already. In fact, we don't end up using these DB keys at all anymore, given how #18 turned out. ;) So, I'm going to fix this to look more like D7's hook_permission()... Stay tuned.

dww’s picture

Fixes #18.E as explained in #19. This is definitely cleaner...

dww’s picture

Fixes #12.B by adding a 'delete' operation and confirm form. Partly addresses #12.C since this delete link doesn't appear for the project owner (and if you try to visit the delete page directly by altering the URL manually, you're redirected with an error message). Still doesn't auto-select and disable all the checkboxes for the project owner's row, but we're a bit closer on #12.C, too.

mikey_p’s picture

F) The notices in #16 are still present

G) When creating a new project, no record is made for the project owner (the tests found this. yay!)

dww’s picture

Ugh, I realized that having applied the D6 simpletest backport patch had screwed up my site's error reporting. ;) I'm seeing notices again, and finally saw the ones you reported in #16. New patch fixes those with the following (from the interdiff):

-  $output .= theme('table', $header, $rows);
+  // Although using named keys in the $header array makes this form easier to
+  // alter, theme_table() freaks out if the $header array has non-numeric
+  // keys. So we ditch the keys at this point to avoid notices.
+  $output .= theme('table', array_values($header), $rows);

This patch also fixes (G). Way to go tests (and mikey_p)! ;) It's also a hell of a lot closer on 12.C, since every time a project is created or edited (in case the owner is changed), the owner is granted full permissions. So, now all we have to do for 12.C is special case the owner row to disabled all the checkboxes.

Also, hunmonk pointed out in IRC that there's one other thing to fix in here:

H) The DB updates assume that there are no bad records in the {cvs_project_maintainers} table and we'll never get duplicates from the project owner vs. the CVS access holders. However, due to the dumb way we treat project owners in the {cvs_project_maintainers} table, there can be cases where we'll have duplicates. So, we should defend against this in some way. Should be easy, stay tuned.

dww’s picture

And lo, (H) is quite trivial. I setup my site in such a way that it'd have the extra records in {cvs_project_maintainers} and ran the updates from patch #23. They did in fact fail. Then I tried with this patch, and they all worked as expected. So (H) is done.

Therefore, I'm just down to finishing the UI for #12.C to prevent anyone from trying to remove permissions from the project owner.

dww’s picture

Status: Needs work » Needs review
FileSize
3.03 KB
35.15 KB

Whoot. ;) This fixes #12.C, too. We now disable the checkboxes for the owner row. Just to be extra paranoid, the submit handler also forces the project owner to be saved with full permissions.

A few other minor UI fixes:

- Always put the owner row at the top of the table.

- Otherwise sort the table by username (by sorting the $node->project['maintainers'] array when loading it -- perhaps a hack but it seemed easier to do it there, and it seems potentially useful to have that array already sorted, anyway).

The only other thing I'm concerned about in here (other than seeing the tests which mikey_p is working on) is this:

I) To defend against a few edge cases, I ended up writing the hook_project_maintainer_update() functions to notice if they failed to update anything and treat it as an insert, instead. So, I'm really wondering if there's any value in separate hooks here, or if we should just merge _add() and _update() into project_maintainer_save().

mikey_p’s picture

I haven't further tested the upgrade functions since I don't have cvs module running on my test site, but here are some tests that cover the access checks for the page callbacks, the maintainers form, and updating the maintainers form, deleting users, etc. They may be a little verbose, but they seems to run fairly fast, and I'd rather be thorough than clever with tests.

Just a note, I didn't get tests for project_release in here yet

mikey_p’s picture

It seems the bot can't handle filenames with #?

Try again.

mikey_p’s picture

Status: Needs review » Needs work

Not sure why the tests either a) aren't running or b) aren't being reported.

Just to followup on the IRC discussion, merging the add/update hooks into a save hook makes plenty of sense from a data consistency point. Adding a load hook for this would be helpful in the fact that one would be implementing a feature specific hook instead of the general nodeapi hook, but I could go either way on this one.

dww’s picture

This fixes 25.I by merging the _add() and _update() hooks into a single _save() hook.

Still needs work for the new thing we've been discussing in IRC which mikey_p mentioned in #28:

J) We're currently forcing any module trying to add a per-project permission to use hook_nodeapi() to correctly update $node->project['maintainers']. Not only is this a bit fragile, hook_nodeapi() is a bit more confusing to get right than a clear stand-alone hook. Plus, I could imagine situations where we'd want to load all the maintainer info for a project without doing a full node_load(). So, it seems we should add a new hook, something like:

hook_project_maintainer_project_load($nid, &$maintainers)

p.s. Attached patch and interdiff doesn't include tests. mikey_p says he's got some typos to re-roll for, anyway.

dww’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
5.87 KB
36.36 KB

Fixes 29.J. The PHPDoc should be pretty clear. Let me know if not. ;)

This is basically done, awaiting tests...

mikey_p’s picture

Here's a combined patch with tests for project, I still haven't made it to project_release, but it could probably extend ProjectMaintainersTestCase to get the assert methods that use name instead of id, and the disabled assertion method.

Status: Needs review » Needs work

The last submitted patch, 69556-project-maintainers-combined-with-tests-31.patch, failed testing.

dww’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
44.71 KB

Yay for tests! #31 found a real bug. I missed a few spots to merge _add() and _update() into _save() inside patch #29.

This fixes that. I also added a few more assertions in the tests from mikey_p. After we grant 'administer project' but before 'administer project maintainers', we try to load the node/N/maintainers tab again to make sure that just having 'administer project' doesn't accidentally grant you access to the maintainers tab. Also, whenever we're testing node/N/maintainers to make sure the user is correctly denied, I also added a test for node/N/maintainers/delete/U which should also be denied. Finally, I added an assertion to make sure that if you manually visit the node/N/maintainers/delete/U link for the project owner's uid, it correctly refuses you.

So, I think we're basically RTBC here, other than tests for project_release...

dww’s picture

To facilitate UI review, here's a screenshot of the new "Maintainers" tab with project, project_release, project_issue and cvs all enabled.

dww’s picture

Here are a few approaches for adding a legend explaining what each of these permissions does. We can't just be like D7 core since we'd have to flip the table, but then we'd have separate columns for each user and the table would grow arbitrarily wide.

K) Generate the <dl> directly inside the theme function for the form.

L) Generate the <dl> inside a collapsed fieldset as part of the form itself.

Patches and screenshots of both attached for comparison.

dww’s picture

Based on more feedback in IRC...

- Going with the raw legend instead of the fieldset (I knew drumm would hate the fieldset) ;)

- Renamed 'Administer projects' to 'Administer project settings' to be more self-documenting about what it actually allows. I double-checked -- yes, that's all it's used for. Well, except hook_access('view') on project nodes, but that's all totally wrong anyway and will be flushed as part of #234463: Remove 'access * project *' permissions.

- Changed the weight of the Maintainers tab to match that of the CVS access tab for consistency.

---

Also, I took it upon myself to rename project_check_admin_access() to just project_check_access() since you now pass in the permission you want to check, and it's not inherently about admin access. It's sorta like user_access() except it's for the project-specific permissions. I can't just call it project_access() since that would be used for hook_access().

Anyway, here's a new (hopefully final!) patch, interdiff (from #33), and screenshot.

dww’s picture

Argh, file attachment #fail with Safari. It keeps puking on .png files for some reason. :(

dww’s picture

Heh, just noticed a typo in the description of 'Administer project releases'. Also cleaned up the description a bit on 'Administer project maintainers'.

sun’s picture

FileSize
23.45 KB

UI-only review:

I don't think that those permission headers need to be that verbose and lengthy. Without the repeated prefixes their meaning becomes much more intuitive:

project-permissions.png

Of course, putting legends below the table (or alternatively permission label titles/tooltips, perhaps?) will help users not familiar with drupal.org projects, workflows, and infrastructure.

That said, "project", with or without "Administer" prefix, is very blurry and I have to re-read the legend every time I want or need to understand it... d'oh! Now I see that it was already renamed to "settings" :) Yes, much better.

dww’s picture

Yeah, we could probably remove "project" from all of them. However, I don't want to remove "Administer" vs. "Maintain". For example, your "releases" column and "issues" column do very different things. But yeah, I can see simplifying to remove "project".

mikey_p’s picture

I started implementing some tests for project_release module and I thought it would make the most sense to extend ProjectMaintainersTestCase so that we can inherit the disabled assertions, etc. However this caused simpletest to run all the test methods in both ProjectMaintainersTestCase and ProjectReleaseMaintainersTestCase, which repeats all the tests when they have already been run. I thought about overcoming this by overriding testProjectMaintainerPermissions. I tested this and it seems to work well, but I thought I'd ask for comments before going ahead with that. If I do that, I'd make sure that is well documented with comments, etc.

dww’s picture

@mikey_p: What about making a shared base class that doesn't define any test methods, just the helpers. Then both ProjectMaintainersTestCase and ProjectRleaseMaintainersTestCase can inherit from the common base?

dww’s picture

New patch that shortens the permission names (in both UI and code) to avoid redundancy as per #39.

dww’s picture

Whoops, the tests are going to fail in #43 since I forgot to rename the perms in the tests, too.

sun’s picture

+++ project.module	16 Aug 2010 20:16:48 -0000
@@ -567,7 +586,16 @@ function project_edit_project_load($nid)
-function project_check_admin_access($project, $cvs_access = NULL) {
+/**
+ * See if the current user has the given permission on a given project.
+ *
+ * @param $project
+ *   The project to check access against. Can be either a numeric node ID
+ *   (nid) or a fully-loaded $node object.
+ * @param $permission
+ *   The string representing the permission to check access for.
+ */
+function project_check_access($project, $permission) {

When renaming this function, I'd recommend to rename it to project_user_access(), as it is like user_access(), just with a context.

Powered by Dreditor.

dww’s picture

Yup, good call. I like project_user_access() for this.

dww’s picture

BTW, here's another screenshot of the UI based on patch #46.

Project maintainer UI from patch #46

sun’s picture

FileSize
66.47 KB

Thanks! Final remarks from me:

project-permissions-2.png

Regarding the first/green: It's probably sufficient to "highlight" these permissions in the legend a bit. Perhaps similar to D7's common security warning for superuser-permissions.

dww’s picture

@sun: Thanks for the review.

Re: super user -- well, "Administer maintainers" does let you grant yourself more powers, so yeah, that's the "super user" perm as such... Not sure we need an extra visual cue about this, but I'm open to being convinced otherwise.

Re: "Maintain issues" meet me (and perhaps wonder95) over at #880820: Add 'Maintain issues' checkbox to the project maintainers form

Re: "Administer CVS" meet me at #880818: Remove CVS access tab in favor of the project maintainers form

Thanks,
-Derek

Bojhan’s picture

Sun is right, I would really change almost all labels. Except maybe settings.

mikey_p’s picture

Setting to needs work since project_maintainers_form() is still missing it's documentation in the latest patch from #46. I haven't tried to fix that in this patch, but this includes fairly thorough tests for project_release.

dww’s picture

Status: Needs review » Needs work

@mikey_p: Thanks, mostly looks great! I'll re-roll for the missing PHPDoc on project_maintainers_form(). One minor question about the release tests:

+    $errors->messages = serialize(array($this->randomName(), $this->randomName(), $this->randomName()));
+    $success = drupal_write_record('project_release_package_errors', $errors);
+    $this->drupalGet("node/$release->nid");
+    $this->assertText(t('Packaging error messages'), 'Packaging error messages shown correctly.');
+    foreach ($errors->message as $message) {
+      $this->assertText($message);
+    }

You need to unserialize $errors->messages before you can foreach() over it...

And lo, running the tests locally confirm this. ;)

Undefined property: stdClass::$message Notice project_release.test 77 ProjectReleaseMaintainersTestCase->testProjectMaintainerPermissions()
Invalid argument supplied for foreach() Warning project_release.test 77 ProjectReleaseMaintainersTestCase->testProjectMaintainerPermissions()

I'll just fix that as I re-roll for the PHPDoc. Otherwise, thanks for the tests!

p.s. What's up testbot, why are you intermittently not running any tests and telling us they all pass? :(

dww’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
53.19 KB

This fixes the test error I mentioned in #52 and adds the PHPDoc we noticed was missing/incomplete.

There's a lingering PHP notice when running the release tests, but that's unrelated to this patch or these tests.

sun’s picture

dww’s picture

@sun: Thanks for the link -- I replied there.

dww’s picture

Status: Needs review » Needs work

Bojhan reviewed this and we discussed in IRC. I'm going to rename 'Administer settings' to just 'Edit project page' since that's what it really gives you at this point. Otherwise, he was happy (given my change over at #880818-4: Remove CVS access tab in favor of the project maintainers form for s/Administer CVS/Write to CVS/).

dww’s picture

Status: Needs work » Needs review
FileSize
7.32 KB
53.06 KB
dww’s picture

Hrmph. The updates ran fine on scratch.d.o. However, something in the bluebeach CSS is making the UI look worse than it needs to -- the table headers won't wrap, so the table becomes unnaturally wide. See attached screenie. If anyone wants to see it in action, visit a project you own on http://scratch.drupal.org and see for yourself.

sun’s picture

FileSize
37.61 KB

project-permissions-3.png
Looks nice for me, but not everyone has drupal.org unleashed ;)
...
I'd recommend to suppress the left sidebar.

dww’s picture

Fixed the CSS in bluebeach. ;) I think this is basically ready to go live.

dww’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests

Committed, merged into bzr, deployed on d.o. DB updates ran without error. Hurray!

Status: Fixed » Closed (fixed)

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

dww’s picture

Here's a screenshot of the maintainers tab on the Project module itself (for possible use in a d.o post about redesign progress).

sun’s picture

Just wanted to follow up once more and say a HUGE THANKS for this! I've had the first use-case for this already. :) (Also, can't wait for the upcoming branch access permissions ;)

Pasqualle’s picture

this is simply awesome

dww’s picture

FYI: It seems #25.I wasn't a good idea, and some people need to tell the difference between a new maintainer and an updated one. ;) See #1171828: Let other modules know when a new maintainer is added...