Once #986718: Add support for sandbox projects lands, we're going to need a setting to disable releases for sandboxes projects. There are two main parts to this:

1) Display some helpful message on the project release settings form at node/%project_node/edit/releases and disable the form

2) When inserting data into {project_release_projects} from the hook_nodeapi hooks for new projects, this setting should be checked and the correct value for enabling releases should be used depending on the sandbox status of the project.

Files: 
CommentFileSizeAuthor
#11 994260-disable-sandbox-releases-11.patch4.02 KBmikey_p
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 994260-disable-sandbox-releases-11.patch. View
#6 994260-disable-sandbox-releases-6.patch3.97 KBmikey_p
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 994260-disable-sandbox-releases-6_0.patch. View

Comments

mikey_p’s picture

Issue tags: +git phase 2, +git sprint 6

tagging

mikey_p’s picture

Assigned: Unassigned » mikey_p
dww’s picture

Yup sounds great. I'm happy to see this split out from #986718: Add support for sandbox projects since that patch is already getting pretty big. ;)

Thanks,
-Derek

dww’s picture

Issue tags: +sandbox projects

adding tag

dww’s picture

Status: Postponed » Active

#986718: Add support for sandbox projects landed. This can now happen.

mikey_p’s picture

FileSize
3.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 994260-disable-sandbox-releases-6_0.patch. View

Here's a start at this, I think we're fairly close, but this is fairly dependent on the UI over at #986718: Add support for sandbox projects so as that gets refined we may need to make a few changes here as well. For now, here's what this patch does:

A) Adds an option to admin/project/project-settings for setting the default behavior of releases

B) Alters the nodeapi_insert hook so that the above setting is checked and the correct value is used when created a new project if it is a sandbox

C) Alters the node/%project_node/edit/releases access callback to check to see if releases are disabled for sandboxes and if the project is a sandbox, and hides the releases tab

D) Adds a checkbox to the project promote form if releases are disabled for sandboxes that shows the current status of releases for the project

E) Adds a submit handler to update {proejct_release_projects} when submitting the promote form

mikey_p’s picture

Status: Active » Needs review

needs review

eliza411’s picture

Issue tags: +git sprint 7

Tagging for Git Sprint 7.

eliza411’s picture

Issue tags: +git sprint 8

Tagging for Git Sprint 8

dww’s picture

Status: Needs review » Needs work

Mostly looks good. A few nits:

A) These functions need the word "alter" in their names:

+  if ($form_id == 'project_settings_form') {
+    return project_release_project_settings_form($form, $form_state);
+  }
+  if ($form_id == 'project_promote_project_form') {
+    return project_release_project_promote_form($form, $form_state);

B) Instead of duplicating this whole query, I'd rather just initialize a variable that holds the value for releases in a conditional and then have one query:

+  if (variable_get('project_release_sandbox_disable_releases', FALSE) && $node->project['sandbox']) {
+    db_query("INSERT INTO {project_release_projects} (nid, releases, version_format) VALUES (%d, %d, '%s')", $node->nid, 0, '');
+  }
+  else {
+    db_query("INSERT INTO {project_release_projects} (nid, releases, version_format) VALUES (%d, %d, '%s')", $node->nid, 1, '');
+  }

I generally find queries a bit hard to scan and see differences. I think it makes the code clearer if we just conditionally define $releases in the if and then have a single query with that as the argument.

C) I generally find variables that are named and behave in the negative a bit harder to grok in the code, since you often do double-negative logic with them. I think the code would probably be clearer if the variable was actually called something like project_release_sandbox_allow_releases and the UI was changed to reflect that.

Frankly all of these are just cosmetic optional fixes for code legibility. So, I almost hate to set this to 'needs work'. But, they should be very quick to fix (at least A and B). I leave C up to you, just wanted to raise it but I don't consider it a blocker for a commit.

mikey_p’s picture

Status: Needs work » Needs review
FileSize
4.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 994260-disable-sandbox-releases-11.patch. View

Rerolled this patch, and made all the changes in #10 and fixed a small logic issue in the access control for the releases subtab, where nodes that already have releases enabled show that tab regardless of the setting and sandbox status.

And yes, C is probably a very personal thing, since it seem a bit harder for me to grok now. Doing !variable_get('project_release_sandbox_allow_release', TRUE) seems to be more of a double negative than having the variable be more descriptive, but it doesn't really matter that much to me.

dww’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. By my tastes, #11 is easier to read re: C. ;) Go figure... ;) Anyway, commit at will.

mikey_p’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 6, -sandbox projects, -git sprint 7, -git sprint 8

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