Add a way for maintainers to indicate multiple supported branches

Well, yeah, what you get now, if you have multiple versions of core, and multiple branches for each one, is a table that looks like the attached screenshot (it's the one for signup, which has 2 branches for 5.x).

That won't work for the whole new UI, but it might work for the "Recommended version" part of it. We're still only going to have 1 recommended version for each version of core, as per http://groups.drupal.org/node/7440 :

After thinking about it more, I believe there's still value in the notion of a single recommended release for each core API. However, this needs to be more strictly defined. It should only mean "what version the maintainer thinks new users should download". So, this will be the single release listed with the project teaser on the various browsing pages. This will also be marked as such in the project node and update_status UI. However, update_status will NOT automatically warn you to upgrade if you're running a different version. update_status will only warn you if your release is specifically marked as insecure, abandoned, or unpublished.

So, in addition to the screenshot of what's there now, here are 2 proposed UI mockups.

First is a "2 tables" approach, where you have 1 table to define the supported major versions for each version core, and then a 2nd table, almost exactly like the existing table, to define the recommended version for each version of core that has any supported branches. You only get a drop-down if there are multiple supported branches. Since there are no supported major versions for 4.6.x, there's no row for 4.6.x in the "recommended versions" table. However, since there are 2 supported branches for 5.x (5.x-2.* and 5.x-3.*), you get a dropdown to select which one is recommended. For 6.x and 4.7.x, since there's only 1 supported branch for each, there's no dropdown -- the recommended has to be the only one that's there...

The final image is a (IMHO, pretty crazy) version where you get a single table for each version of core, and you define both supported and recommended in the same table. It's wonky, but at least it's here as a straw-man proposal for feedback, and there might be elements of it that inspire a cleaner approach.

Here's a (perhaps) better version of the 1-table-per-core approach. Since there's only 1 currently recommended version per core, it shouldn't be in the table at all...

Thanks for starting this. A few implementation gripes:

A) s/core/api/ -- "core" is specific to d.o. in project_release, internally, we always call this the "api compatibility term", $api_tid, api_vocabulary, etc, etc.

B) Incorrect use of t(): array(t('Major version', 'Supported'))

C) theme function doesn't deal with 'Recommended' at all.

D) seems like the advanced settings (which have nothing to do with the recommended/supported data structure) should be handled directly in project_release_project_edit_form(), and the helper form builder function should be specific to the crazy table UI stuff.

E) _project_release_project_edit_form_getdef() should be renamed to something more self-documenting, like _project_release_get_version_major_data() or some such. Obviously, that function is bogus, and needs a db_query() to figure that stuff out. A // TODO comment to that effect would be nice. ;)

F) '#core_name' is used in the theme function but no where else...

But, this is certainly the beginnings of a great patch. ;) Thanks!!

New patch with project_release.install changes for schema update (tested on MySQL, coded for pgsql, too). Otherwise unchanged from before.

Upon further thought, we should probably let maintainers specify dev snapshots per major/branch, not just a single bool for the project. I hope this doesn't make the UI too busy/confusing -- I think it works. If we go this route, we'll need more schema (and code) changes, but I think it's worth it.

Whoops, a few bugs in the previous mockup. Use this instead.

@drewish: As far as project_release is concerned, what matters if the version info in {project_release_nodes}, not what branch/tag each release node happens to be pointing to. The evil HEAD release nodes from before the new release system don't really factor in at all, since they don't have valid version info. They don't show up on the project node, nor the project browsing pages. Only once they have a "Drupal core compatibility" term associated with them, and real version info, do they matter... Whether or not a release node is pointing to HEAD or another branch is irrelevant. All that counts is if that release has valid version info or not.

@all: Here's a new patch for project_release.install that includes a (completely untested) DB update to move the snapshot stuff from per-project to per-branch. It's also smarter about populating the new table with rows for the non-supported majors, too. I think it should work, but it definitely needs some good testing, e.g. on project.d.o or scratch.d.o. As currently written, it's non-destructive (the queries to DROP the old table and column are commented out), so we could run it on any of these sites, see if the new {project_release_supported_versions} table is created and populated correctly, and if not, DROP the new table, fix the DB update, and try again. ;)

Still no more changes to the rest of the code, I was just thinking about the DB upgrade path and the mockups, so those are the main things I've been working on...

Finally tested the upgrade code a little, this patch fixes some SQL errors I had, and also improves the code to populate the non-recommended majors into the {project_release_supported_versions} table such that we LEFT JOIN on {project_release_default_versions} to avoid duplicates. This still needs better testing, especially on a copy of the d.o data, but this is a lot closer, and might be all we need.

Tried testing the DB upgrade on p.d.o. Found a few problems. New patch that's just against project_release.install...

Ok, this one really works. ;) We need to ignore release nodes with NULL as version major (the evil ~1000 HEAD release nodes from the conversion to the current release system). The {project_release_supported_versions} table is now populated correctly on project.drupal.org (not that anyone else can see that or play with it yet). ;) This patch also includes dmitri's initial efforts in project_release.module again...

Started working on the code itself again, based on my review in #5:
A) TODO
B) Fixed
C) TODO
D) Fixed
E) Fixed
F) My review was bogus, I see where you're setting it now. However, '#core_name' is wrong. This should be something like 'term_name', or 'api_term_name', as per point (A). I went with 'api_term_name', so we can call this Fixed.

whoops, that last attachment was the wrong patch. Oh well, it's obsolete now anyway...

New patch that fixes (A) and (C). Furthermore, this implements the form builder, theme function, and submit handler, for the primary form elements: supported, recommended, and snapshot. Also includes the help text. See attached screenie.

TODO:
G) The validation handler.

H) Should the help text continue to live in hook_help() or is it better as a form element so it's more easily altered?

I) Display the 'Currently recommended' version based on the saved settings.

J) JS to update 'Currently recommended' based on current form values.

K) Do we need better visual explanation of the '6.x', '5.x', etc?

L) Do we want some kind of header above the help text, like "Supported versions" as in the mockup? If so, that speaks in favor of deciding (H) in favor of a markup form element, not hook_help().

Just installed the latest patch on http://project.drupal.org. Obviously, there's still no good validation for the sanity of your choices, and this issue is *not* about the resulting changes to the project release download table (which lives at http://drupal.org/node/176776), so there's not much you'll be able to see, yet, other than the admin UI itself. However, folks can start to play with it there if they wish.

This also made me think of another TODO item:

M) This UI should somehow respect the 'project_release_active_compatibility_tids' setting.

See http://drupal.org/node/204140 about updating project-release-create-history.php to take advantage of this new data. A volunteer for that would be most welcome.

FYI: http://drupal.org/node/204140 is now done and installed on p.d.o. Not yet committed to CVS, since I want to wait until this is RTBC, too...

Another TODO:

N) Cleanse the entire project_release.module of references to {project_release_default_versions}

I'm working on this now...

Untested, but this should deal with (N) completely.
I guess I'm working on the rest of this now, so assigning to myself...

Whoops, that was broken. ;) This is better.

Initial stab at the validation handler. Sadly, something is screwy here, and even though the messages show up properly, the form elements themselves aren't getting flagged correctly. :( I'm trying to debug this with chx and eaton in #drupal right now, but I wanted to at least post the work-in-progress patch here in the mean while...

This patch incorporates all of aclight's suggestions from #18. The revised help text is moved into a form element (H) along with a header for this section of the form (L).

Fixes (M) by honoring the 'project_release_active_compatibility_tids' setting.

Adds support for displaying the currently recommended version in the table (I).

Turns out the validation stuff is all working correctly, it's just that most browsers don't handle the red boxes around the checkboxes and radios. :( UGH!! So, I'm declaring (G) done, as well.

So, all that remains would be:
(J) the jQuery sugar coating to update the current recommended as you change stuff
(K) better visual explanation of each core compatibility term ("5.x", etc)?

These are both minor cosmetic points, so I'm setting this back to needs review. The attached patch here is already installed on http://project.drupal.org and seems to be working nicely given my testing. It even correctly updates what's displayed on the front page of the project nodes based on the current recommended version (it just doesn't display everything that's marked as supported at this point, I'm leaving those changes for http://drupal.org/node/176776).

Ugh, one more real problem with the current UI:

O) If you uncheck all the "supported" checkboxes for a given version of core, there's no way to "unselect" the radio entirely, so the validation always fails.

So, I think I need to special case this in the validation and submit handler. If a given api tid has no supported versions, just ignore the other form values and clear everything to 0s...

New patch that solves (O)... I think this is all we need for heavy testing for http://drupal.org/node/197186.

Indeed. ;) Thanks for mentioning it. I'll have to take a look.

Whoops, I already had that fixed in my local workspace, it's just that the latest patch was missing the (minor) changes to project.module and only included project_release.*. This patch is now installed on p.d.o and working fine. ;) Phew.

Slick! starbow was kind enough to email me some JS goodness to automagically update the "currently recommended" based on the current value of the radio. now installed on p.d.o. looking sweet! THANKS! ;)

I've got a few other ideas for JS goodness for this UI. Basically, it'd be nice if the "recommended" and "show snapshot releases" columns were automatically cleared and disabled whenever the "Supported" checkbox for a row isn't checked. This is enforced via FAPI validation (of course), but it'd be slick if JS made the rules more obvious directly in the UI. See what I mean? Not sure how hard this would be, but it seems like it should be pretty easy for the JS-enabled among us. ;)

Actually, the DB upgrade could use a little work. It generates some invalid states where the "snapshot" columns are checked for major versions that aren't supported...

That was easy. ;)

WOOT! ;) /me hugs starbow again. ;) Here's the patch with the line-feeds cleaned up. Now installed on p.d.o. This is looking amazing.

Ugh, whoops. Now the auto-updating of "Currently recommended" is broken:

http://project.drupal.org/node/29351/edit/releases

It's always telling me about 5.x-2.2, even if I try to make 5.x-1.* recommended (unless I actually save the form with 1 selected, on the reload, it correctly tells me that 5.x-1.0 is currently recommended).

Also, if the supported checkbox *starts* as unselected on page load (due to saved settings), we should disable the recommended radio and snapshot checkbox for that row, too...

Much better, thanks! I made a few more changes myself:

- fixed typo in one of the validation messages.

- more verbose validation messages that at least include which version of core the message relates to. since most browsers fail to display the nice red boxes around the error checkbox or radio elements the form_set_error() is marking, at least the text can help point people to the right place to fix the error.

- in local testing, i found that i needed to add this to function _project_release_get_current_recommended_version():

drupal_set_header('Content-type: text/javascript');

otherwise, with devel.module enabled, you get the huge query log table junk printed at the bottom. i know this isn't technically javascript itself, it's just a plain string. so, the alternative would be this:

$GLOBALS['devel_shutdown'] = FALSE;

i'm not sure which is better. the header is more generally useful, but not exactly true. the devel_shutdown global solves this particular problem, but is more special-case. @starbow: any opinions on this?

Installed #41 on p.d.o, and it's looking very good -- nearly perfect. However, I noticed 1 lingering problem with how the currently recommended stuff is handled when you uncheck a supported checkbox. e.g. start here:

http://project.drupal.org/node/29351/edit/releases

- for 5.x, 1 is supported and recommended, and currently recommended == 5.x-1.0
- if you uncheck "supported", the whole row is cleared out and disabled, but currently recommended stays as 5.x-1.0
- then, if you click "update", it tells you "You can not recommend a major version that is not supported for 5.x.", and you see that the recommended radio is actually still selected for 5.x-1.*. it's as if the ".removeAttr('checked');" in the JS doesn't fully work to clear the radio -- it just visually clears it but under the covers, it really still behaves as if it was selected as 5.x-1.*. :(

I tried this on both Safari 3 and FF 2.0.0.11 and it's broken the same way for both browsers, so I'm pretty sure this isn't just Safari being stupid.

Instead of just using ".removeAttr('checked');", would it be possible to explicitly set the value of the recommended radio to "-1" or something?

Cool, I think that does it. Attached patch is now installed on p.d.o, and it's working reliably for me, too. Woo hoo! Thanks!!

Sorry, spoke too soon. ;) It's still a little wonky in the case where you save it. The scenario I describe above seems to consistently fail on both safari and FF. I'll try again to spell it out:

A) Start with 5.x, both 2 and 1 are supported, and 1 is recommended. (project_203313_js_A_initial.png)

B) Uncheck "Supported" for version 1. (project_203313_js_B_unsupported-1.png)

C) Click "Update" and you get a validation error: "You can not recommend a major version that is not supported for 5.x." (project_203313_js_C_update.png) Note here that "Recommended" radio is selected for major version 1, but disabled since the "Supported" checkbox isn't checked.

Are you not seeing this?

Thanks for all the help in here. This is going to totally kick ass when it's done and deployed on d.o. ;)

@starbow, thanks for continuing to work on this complicated mess. ;) I was out of town all weekend, and just now am getting back to this issue. I rerolled your latest patch to strip line endings, fix it for 'Content-Type = text/plain', and remove the bogus // $Name: BASE $ changes. now installed on p.d.o and looking great to my testing. my previous failure case is now handled splendidly. ;)

However, I'm going to leave this CNW, since I agree, we should probably just rip out the AJAX entirely, and have the form builder just populate the form with the recommended version from each branch in a hidden column. then, the js can just twiddle things to display the right one based on the recommended radio.

should I just work on a new patch to everything else and upload it here, then leave the project_release.js changes for you? would that be the easiest approach? yes, i think multiple calls to project_release_get_current_recommended() is fine. any attempt to get that info correctly in a single query is just going to make a vastly more complicated and expensive query. plus, i can't see how N queries like that for the form builder for this relatively obscure maintainer UI is going to be a problem, particularly if it saves doing another query to handle the AJAX request on nearly every click on the UI. ;)

Something like this for the FAPI code, I guess. I ripped out the AJAX menu handler, too. This just needs changes to project_release.js to populate the visible "Currently recommended:" field using the 5th column (of hidden form elements) in the table. Not sure if this is best, but I figured I'd get it started down this route. Please let me know if there's a better way to do this. Thanks!

@starbow: I'm confused by your latest patch... it seems to be identical to your last one from #48, and therefore undoes whatever goodness I added in #50. Did you just upload the wrong thing? Thanks.

Btw, yeah, I agree the hidden column shouldn't be a real table column, I just wasn't sure the cleanest, easiest way to handle that, so it was a start. I'm anxious to see what you cooked up that's better... I'm always learning about all this JS mojo and finding better ways to deal with things. ;)

Cheers,
-Derek

Cool, almost there. I spotted 2 bugs in initial testing:

A) The "Currently recommended" field isn't filled in initially due to a name mismatch in the form array.
B) The "Currently recommended" value isn't set to the yellow background when you change it, unless you completely unset it.

I can definitely fix (A), and I think I see how to fix (B), so I'll take a quick stab at re-rolling this. ;) Stay tuned.

New patch with the following:

- Correctly populates the initial value for $current_recommended by looking for 'version_name' in the $form array (what it's now called), not 'current_recommended'.

- Correctly handles the case where the initial value should be 'n/a' when there's no supported or recommended versions for a given version of core. My previous db_query() here was assuming a valid result, but we need to handle a db_result of 'false', too.

- project_release.js both updates the version string and sets the yellow background when you toggle the recommended version. starbow, if you could verify I did that the elegant jQuery way, that'd be swell, but I think I did it right. ;)

Now installed on p.d.o. Seems to work great in my testing. ;) PHEW!!! ;)

The *only* other UI improvement I could see here is setting yellow backgrounds for *any* unsaved changes, not just the recommended version (e.g. the snapshot or supported checkboxes, too), but I don't know if that's worth the hassle.

I think this is dangerously close to RTBC now. Any other reviews or tests would be welcome. Thanks!

Drat. :( The jQuery part of the UI is RTBC, but under heavy testing of the kinds of edge cases we've been seeing at http://drupal.org/node/126554, the underlying code, especially project_release_check_supported_versions(), is wrong. :( I'm working on a patch now, stay tuned...

Like so. Massive re-write of project_release_check_supported_versions(). This also incorporates the input from http://drupal.org/node/126554#comment-696383. I tested this *A LOT* locally, all kinds of edge cases. I couldn't seem to break it. Now on p.d.o, if people want to try to break it themselves.

Fixed 1 more minor UI bug. When we were selecting the API terms to generate our form, we weren't specifying an order. Now we ORDER BY term_data.weight, term_data.name, and everything works as expected. Installed on p.d.o...

Even better. Instead of calling taxonomy_node_save() ourselves in the case where that mattered (a new release node is created directly as a published node), we can handle everything elegantly and in one place by implementing hook_nodeapi for 'project_release' nodes. Since project_release has to be weighted heavier than taxonomy for other things to work, anyway, we know this nodeapi hook will fire after taxonomy's. Therefore, on new published release nodes, the term associations will already be in the DB when we hit project_release_check_supported_versions(). This actually simplifies some other code, too, since we don't have to handle this checking code (and cache clearing) separately in hook_update(), hook_insert(), and hook_delete(). Yay.

Now deployed on p.d.o. This definitely solves all the edge cases reported at http://drupal.org/node/126554 under my testing. I think this is now totally RTBC. The only reason to wait is that it'd be nice to see progress in http://drupal.org/node/176776, since I'd like to deploy both at the same time. However, I wouldn't even mind a brief transition period where only the recommended version from this maintainer UI is presented on the project node, and the 'snapshot' checkbox doesn't really do anything (which is how it works now). That said, any more testing or reviews would be most welcome. Thanks.

Committed to HEAD. Yay! I'll mark #126554# fixed once this is deployed on d.o. Thanks again to everyone who helped!

Whoops, there was a lingering spot in project_release_project_nodeapi() where we were trying to populate {project_release_projects} with the now bogus "snapshot_table" column. Now fixed in HEAD: http://drupal.org/cvs?commit=96837