Add support for sandbox projects

I originally suggested doing this in Project* in my IRC chat with mikey_p, so I'm obviously in favor. ;) I think everything in the above list is a reasonable feature for Project* itself to support internally. Once we have a 'sandbox' bit in the {project_projects} table, we could even have global settings for how sandbox projects behave:

Disable releases for sandbox projects [x]
Disable issue queues for sandbox projects [ ]
Exclude sandbox projects from solr search index [x]
...

Obviously, each would be provided by the module handling the feature (e.g. project_solr would manage the setting about excluding sandboxes from solr search index, project_release would handle releases, etc).

Doing it first-class in project* also makes it trivial to have separate permissions for creating sandboxes, creating non-sandboxes (need good terminology here), and for toggling the status. Maybe we just lump the toggle the status permission into 'administer projects'...

Anyway, I think this makes tons of sense for a lot of reasons. The only real downside is that it's another custom "field" in project*, and we're trying to move away from those. But, a) the 7.x-2.* series is a long way off, and b) we can easily make this a core field when we get there, and 85% of the related code won't really have to change.

So, unless there are compelling objections to this plan, I think this is way better than a d.o taxonomy vocab for this and a whole pile of custom d.o-specific code.

Cheers,
-Derek

p.s. This isn't a bug, but it's related to Git phase 2...

subscribe

I hear the sandbox stuff is some of the first of what you want to work on, so marking for sprint 6. I'll also try to put some brainspace in on this, probably later this week.

Looking great, thanks!

A) s/$sandbox_value/$is_sandbox/ ?

B) We need a description for sandbox checkbox (as you pointed out above).

C) Both inner cases are the same:

+  if (variable_get('project_sandbox_numeric_uri', FALSE) && $sandbox_value) {
+    if (empty($node->nid)) {
+      $form['project_node']['project']['uri']['#disabled'] = TRUE;
+    }
+    else {
+      // @TODO, would be nice if this was just a disabled item, so that the js could toggle this when the sandbox is checked off
+      $form['project_node']['project']['uri']['#disabled'] = TRUE;
+    }
+  }

I don't really understand the @TODO in there anymore.

D) I don't think we want a 'project_sandbox_permission' setting. And now that I think about it, we don't want to re-use 'administer projects' for that, since we wanted to basically give "approved" contributors the ability to create non-sandbox projects on their own without a moderation queue. So I think we need a new "modify project sandbox status" permission (more or less).

Or, perhaps we want to stick with the permissions I sort of outlined up in #2. Basically, we just add a new permission called 'create sandbox projects' and finally rename the (inconsistently named) 'maintain projects' permission to just 'create projects'. We only give them a choice to toggle the sandbox if they have both permissions (or if they have 'administer projects' I guess). Not exactly sure what'd be the most obvious and self-documenting, but I think this needs more thought. Certainly we need a way to say "role X can only create sandboxes" while "role Y can create sandboxes or real projects, but can't alter other people's projects" vs. "role Z can alter other people's projects and promote them from sandboxes to real projects".

E) Should we allow demoting real projects to sandboxes? Should that require a separate permission or setting?

F)

+    '#description' => t('If checked, projects marked as sandboxes will be have their shortname automatically generated.'),

". . . using a unique numeric identifier" ?

G) The form code and nodeapi() stuff to enforce 'project_release_sandbox_disable_releases' seems overly complicated (in part by trying to reuse $form['advanced']['releases'] but as type 'markup' in the sandbox case). Why not just make $form['advanced']['releases'] type '#value' that's always 0 in this case (so none of the nodeapi() and submit logic has to change), and just use $form['advanced']['releases_help'] or something for the markup?

Otherwise, this all seems very much on the right track... hurray!

Thanks,
-Derek

p.s. Any chance of .test hunks for any of this functionality? ;)

Side note: I edited the original post to point to the relevant issues for all the desired features of sandbox projects.

Meta: Great, happy to see #994260: Add a setting to disable releases for sandbox projects in its own issue (and I updated the OP here to link to it). Also, this issue isn't a "determine..." task anymore, it's a "do it" task, changing title accordingly. Finally, I'm introducing a sandbox projects tag to track everything related to this.

The patch: definitely getting closer, thanks! Here's what I found on a close review of #12:

H) The permission checks and default values shouldn't be happening on existing projects. For example, someone might not yet have unlimited permission to create full projects, but they can still create a sandbox that was promoted to a full project. When these users edit their project page, we don't want to turn the project back into a sandbox.

I) Why do we only disable the "uri" setting if $node->nid is empty? Don't we always want to disable that form element if sandboxes have numeric shortnames and we're dealing with a sandbox?

J) Speaking of "uri", I've always found the project code confusing as hell with this called a "uri". We're definitely not changing that everywhere in this patch, but I'd be happy to call the new settings related to this *shortname* not *uri* (e.g. "project_sandbox_numeric_shortname") so we don't have to change it later. However, I'm not 100% sure I agree with myself on this point. ;) The other arg is that since it's called "uri" everywhere else, it'd be even more confusing to introduce an inconsistency in the short term, even if the point is to migrate towards better naming in the future. So, I'll leave it up to your judgement on this, but I did want to mention it…

K) Looks like we still need a DB update (could just be part of project_update_6003() to migrate the old 'maintain projects' permission to 'create full projects'.

L) Code style error:

+  );$form['sandbox']['project_sandbox_numeric_uri'] = array(

Otherwise, looks fantastic. Some simpletests would be great. I can't manually test right now, but I'll do that after the next patch.

Re (E): Hmm, good question. ;) It's related to what I'm talking about in (H). Basically, we need all that code (and UI) to know the difference between a new project (where the code you've got in #12 applies perfectly) and updating an existing project (which needs separate logic). In the update case, I think we've got the following possibilities:

1. Sandbox project, user doesn't have 'create full projects' nor 'administer projects': Force it to remain a sandbox
2. Sandbox project, user has either 'create full projects' or 'administer projects': Default to remain a sandbox, but allow it to be changed
3. Full project, user doesn't have 'administer projects': always disable the checkbox and force it to remain full?
4. Full project, user has 'administer projects': default to full, but let the user demote?

Not 100% sure about 3 and 4 just yet... Your input is welcome. ;)

Thanks again, this is going to be great when it's deployed!

Cheers,
-Derek

p.s. See also #994552: Support path redirects when project shortnames are changed

Argh, WTF? I just noticed the text bot's comment #13 clobbered the issue tags in here. That seems incredibly scary. :( Wonder what PIFR bug is causing that...

Getting very close now. I've tried using it on a local site and ran into a few things that are a bit confusing in the UI. Otherwise, this is nearly RTBC.

M) If the 'Auto generate short name for sandboxes' option is enabled, I think it's confusing to have a disabled 'Short project name' field when creating a new project. If the sandbox option is checked (either b/c that's the only option the user has or dynamically if they have power to choose) I think it'd be better to just completely hide that form element instead of disabling it. Just makes things confusing (and in a way, frustrating as a user) if they see a field but can't click on it or do anything with it. When editing an existing node (that already has a value) it's great to display that as a disabled field. But on new projects, it's weird. For example, if you can create both, and the form defaults to not creating a sandbox, you might fill in a short name, then check the sandbox field, then the value you typed is there but disabled, but then when you submit it, it's ignored and reset to the nid anyway. I think it'd make a lot more sense to just completely hide this field on new projects if they're going to be a sandbox project. Shouldn't be that big a change to the code, but I think it'll be a big improvement to the UI.

N) I was testing stuff as a user with 'administer projects' but not 'create full projects' permission. I expected to be able to edit a project and promote a sandbox to a full project, but I wasn't able to. Maybe 'administer projects' is a weird permission that needs more thought. But, at least for now, I'd expect if I had that permission, I should be able to manipulate the sandbox status. So, I think we need to revisit the logic to take this permission into consideration.

O) Given that there's no turning back once you promote a sandbox to a full project, I'm thinking we need a confirmation form for that action. Maybe that's going to suck too much (especially given that this is a node form). :( But, it seems like we need *something* to get the admin (or many project author, really -- anyone who can create full projects, which we expect to be a pretty large pool of people) to go a bit more out of their way. Promoting a project from sandbox to full is a Big Deal(tm), and we don't want that to happen accidentally. I know this is complicating things, and maybe this is the wrong approach, but perhaps if you're editing an existing sandbox project, and you have permission to promote it, instead of just enabling the checkbox, we instead display a "Promote this to a full project" link that sends you to another form entirely so that we can provide more help text about the implications, we have a good place to hook into things (e.g. to solve stuff like #991504: Ensure Git repository names match project shortnames across transition from "sandbox" to "real" projects on d.o), and we can even add another confirm form step without screwing up a node form (although that might be overkill if we're already on a separate form). If you're able to create both sandboxes and full projects, I think it's fine to just leave the checkbox on the new node form, since it's not as catastrophic to just create something as a full project in the first place as it is to convert something that really should be a sandbox into a full project.

P) Obviously, you're waiting for the exact behavior we want to settle down before you write tests, which makes perfect sense. But yeah, tests would be great. ;)

Thanks!!

Another problem I ran into that I wanted to split into a separate issue is here:
#997906: Reconcile project_allow_uri_update == FALSE and sandbox projects being promoted to full projects

That seems like yet another good reason to explore what I propose in #19-O about splitting off the act of promoting a sandbox to a full project into a whole separate workflow...

Re: (P) speaking of tests... there are references to 'maintain projects' inside the existing project.test file, and those create catastrophic failures with this patch applied. ;) So, even if we don't manage to add new tests for this functionality immediately (which we should eventually), we at least need to fix the existing tests so they can keep running.

Q) Note: I don't think we want "configurable" help text here, my bad. So long as it's a form with a unique form_id, I'm happy to say that sites that really need to customize the text use form_alter(). In fact, same for the existing setting in this patch already. We can just have some generic text and then sites can alter if needed. I don't think we want to bloat RAM usage on every page load for this. ;)

Awesome. A few new things in the latest patch:

M) You're only hiding the short name field on sandbox projects once they already exist. If you re-read #19.M you'll see I was proposing to hide that field on *new* projects, and leave it visible (but disabled) on existing ones.

R)

+  $errors = project_validate_project_shortname($node);
+  foreach ($errors as $error) {
+    form_set_error('project][uri', $error);
   }

If there are multiple errors, only one will show. See #936490: Update module should verify downloaded tarballs and propagate errors correctly for more. :(

Otherwise, this is looking fantastic. I've gotta run now, but if you fix these, (and ideally add tests) I think we're there...

Thanks!

I think the promote tab should be accessible if the following are true:
- The project is currently a sandbox
- The user can edit the project node (either b/c they're the owner, they're a maintainer with edit perms, or they're a site admin with 'administer nodes' or equiv.
- The user can either create full projects or has 'administer projects' perms

If the user can create full projects, they could always just make a new project node and copy the sandbox stuff into the new project. So, it seems pointless to prevent people who can create full projects from promoting sandboxes they can edit.

Just reenforces the point we're going to need (at least) two roles for d.o project maintainers:

- Can make sandboxes: we give out like candy once they pass the most basic "I understand my contributions will be GPLed" test.
- Can make full projects: highly trusted contributors who we want to allow bypassing the "please promote my sandbox to something real" workflow

I've been working closely with mikey_p on all these revisions, closely reviewing, and testing. There are a few spots that could use a UI review, and we definitely need tests, but at this point, I think this patch is good to go. And I've recently given mikey_p commit access to project*, so this can be the inaugural commit. ;) Yay!

p.s. Once this lands, we need to commit the attached to project_issue.test.

Yaya! Now we just need tests for the new functionality. ;)

Also, I committed my fix for project_issue.test from #35.

Tagging for the tests during Git Sprint 7

subscribe

Tagging for Git Sprint 8

Since this is just tests remaining, going to remove it from a formal sprint. Still good to get it done, obviously, but not a blocker for community testing or launch.

Moved tests to a new issue: #1028750: Add tests for sandbox project functionality

Followup of #9:E - #1774584: Ability to demote full projects to sandboxes

Added link for #994260 now that that's a separate issue