Splitting this off from #850142: Integrate VCAPI with project_release node add form since the edit and add code paths and logic are totally different. No sense blocking the node add issue until edit is working. They could even (in theory) be done in parallel, although I'm just going to do this after #850142 is done, since that's much more critical. We'll still need this for launch, but it's a smaller task, and not 100% required for the sprint 8 deadline. It's still my intension to complete this during sprint 8, so I'm tagging accordingly.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 1019294-5.vc_release_node_edit.patch | 10.49 KB | dww |
| #5 | 1019294-5.vc_release_node_edit.branch-admin.png | 41.7 KB | dww |
| #4 | 1019294-3.vc_release_node_edit.regular.png | 35.45 KB | dww |
| #4 | 1019294-3.vc_release_node_edit.admin_.png | 39.61 KB | dww |
| #4 | 1019294-3.vc_release_node_edit.branch-regular.png | 37.95 KB | dww |
Comments
Comment #1
dwwThis is a start. It's now basically working fine. However, there are some problems in the case where a user has the 'administer projects' permission:
A) Do we even *want* to let admins change any of the low-level details of the release node? Now that we're not allowing HEAD, there's basically no way to mess something up, and no need to fix anything. I guess on a site like d.o there are going to be some legacy release nodes that might need to be repaired. However, given that a somewhat large group of folks has this perm on d.o, and they've been known to mess these things up in the past, I'm inclined to just diable all these form elements and make the "admin" case look like the "regular" case in the attached screenies.
B) If the answer to (A) is "Yes", then there's nothing to validate the new VCAPI label you define (branch or tag), make sure it really exists in the project, update {versioncontrol_release_labels}, etc. If (A) is Yes, we should probably change this UI for admins so that it queries for all available branches + tags, adds the current one, and gives you a drop-down, not a text area. There's a // TODO in the hook_nodeapi() about this, too.
So, if (A) is No, we need work to rip that out. If (A) is Yes, then (B) needs work. However, even though this needs work, I'm setting to needs review to get reviews on the rest of the patch and feedback on these points... ;)
Comment #2
dwwOkay, based on some IRC feedback, the overwhelming answer was No on A. As I pointed out "worst case, Real Admins Have A Mysql Prompt(tm)". ;)
Stay tuned for a new patch...
Comment #3
dwwThis should do it. Tested locally.
Comment #4
dwwHere are screenies of regular vs. admin via patch #3. The only diff is admins can edit the title, regular can't. The tag or branch is locked for everyone, and there are no other crazy fields. #3 also included some logic to hide the version field (instead of displaying readonly) if the VCS label matches the version string exactly. So, I'm also including screenies of editing a branch (where on d.o, the version string is a bit different from the branch name).
We could open a separate issue in drupalorg.module about altering this form again to completely hide this whole fieldset if we deem it's unnecessary for our uses. But, I think I want to leave vc_release with at least the logic in here now.
Actually, looking at all 4 cases, I guess we shouldn't let admins edit the version string itself if they can't edit the parts of it. Trivial fix, one sec.
Comment #5
dwwLike so. Other screenies remain unchanged.
Comment #6
dwwCommitted #5 to HEAD. If we want to do a d.o-specific UI review, let's open a (non-critical, non sprint-8) issue about it in the drupalorg queue. ;)