need to reset component (and other fields) when changing project

that's right. the validate step fails (as it should) since you're trying to move the issue into a new project where the old list of components doesn't make any sense. when validate fails and you hit submit, it returns you to the form, with as many of the values you've already filled in as possible. on the 2nd try filling out the form (that submit becomes a preview in this case), all menus will now have the right values so you can select a valid component. validate will succeed, and submit will work. this is how it should (and does) work.

in theory, we could use ajax/js trickery so that when you select a different project, it automatically changes the possible values in the other fields, but that's a whole other story. if that's what you have in mind, change this to a (minor priority) feature request.

thanks,
-derek

the activeselect module is what we'd want to use for this feature, if/when we ever implement it. ;)

I wanted this functionality for a site I was working on so I went ahead and added support for the activeselect module. It should degraded nicely if the activeselect module isn't installed/enabled.

Also, this patch has a small change to order projects in the dropdown by name (with taxonomy enabled, they're ordered by taxonomy first, then name - without taxonomy, there is no ordering). This can be removed and handled as a separate issue if necessary.

-Ed

thanks so much for working on this!!!

however, please attach a patch file, not a .zip.
http://drupal.org/patch

i'm on vacation until july 16th, and don't have access to a machine where i can unpack .zip files, apply them to a copy of the source, etc. however, if you post a plain text, unified diff file (as described at the above link) i can at least visually inspect the code and provide feedback.

thanks, again!
-derek

Here's an updated file. I added in a few comments, adjusted the code slightly per some of the coding standards, and adjusted the diff parameters I used.

Thanks for the link - I'll bookmarked it for future patches.

-Ed

Looks like I missed a few tabs in the previous patch. This file just converts them to 2 spaces to make things easier to read.

-Ed

yup, that's a real patch. :) thanks. no time right now to review it, but i'll take a real look as soon as i get a chance.

thanks again,
-derek

p.s. technically, we should do the same stuff for the "version" field, too, since that's specific to the project you've selected. not having used activeselect before, i'm not sure if that's more easily accomplished in a single patch with your changes for component, or if it'd be just as easy to handle that separately. please think about it, and if possible, either add support to this patch to handle version (if that's not too much additional work) or create a new issue and patch that adds that independently. of course, that's just extra credit, so don't feel obligated to get this working. :) but since i won't really be able to test/apply this for another few weeks, i figured if you wanted something to work on in the mean time, that'd be a great addition. thanks!

after looking more closely at the patch (but still without a good means to actually apply it and play with it myself), a few questions:

1. pardon my ignorance about activeselect, but why is project_all_project_components() needed? that seems awfully expensive, and counter-intuative that we'd even need it. seems like active select should just be told the current value of project, and it returns the current options for component based on that project. why do we need a complete list of all possible components from all projects?
2. what happens if you're following-up to an issue, there's already a selected value for project and component, you change the project, and the currently selected value for component (from the old project) doesn't exist in the valid components for the new project? i'd hope that activeselect (or this patch) would be smart enough to automatically select "none" for the component in this case, but i didn't notice any code to provide this behavior. i don't exactly follow the handling of the $extra parameter to component_activeselect(). it looks like it's dealing with this, but it's not obvious how it really behaves in this (highly crucial) case.
3. the explode() on '||' in component_activeselect() strikes me a little weird. '||' always seems like boolean logic to me, not a good thing to use as a string delimeter. is this standard practice for activeselect for some reason, or something you just made up for this patch? if it's non-standard please change it. if activeselect likes it this way for a reason, please point me to some documentation that explains why. :)

that's all that comes to mind at this point. thanks again for working on this. i just recently had problems with this exact part of the project module on a live site (since the current mechanism for changing an issue from 1 project to another is very confusing if you don't know how to deal with it). i'll be very happy once i can actually test this, commit it to the project's codebase, and install it on this other site. ;)

thanks!
-derek

Derek,

Here are some answers to your questions:

1. This is added to address one of the requirements for the Activeselect module - that all possible options are included in the dropdown when it's built. Here is the exceprt from the activeselect module's api.txt file:

   It is very important that the values defined in the '#options' array for each target element include EVERY POSSIBLE OPTION that could be outputted by your activeselect callback function. There are two reasons for this. Firstly, if the activeselect module is not present (or if it is present, but the user's browser has inadequate JavaScript support), then the form will degrade gracefully, and the page will still be reasonably usable. Secondly, if activeselect is present (and adequate JS support is available), then whatever values the user submits must be defined in the '#options' array for that element. This is because Drupal checks this presence as part of its default validation routine, and so this validation will fail if the option element cannot be verified.

   I've addressed the first part of the first issue (when the activeselect module isn't present) but that still leaves the second part of that issue (inadequate JavaScript support). I haven't looked into this issue, but assume that the activeselect module developer has.

2. This is handled in the component_activeselect() method. This method always adds <None> as the first option and marks it as selected. The only case where <None> isn't selected is when you switch to a different project, and then back to the original project. In that case, the previously selected component will be selected. The patch is assuming that different projects won't necessarily have the same components. Additionaly, if you are switching projects, this forces you to consciously select a component (which could be argued as a better approach than just assuming if a component has the same name, then the issue belongs under that component). If I have some extra time, I could look at what it would take to make this optional.

   I just noticed a small bug in the code too. Technically, if I'm marking an item as (other than the <None> entry) than I should mark the <None> entry as unselected. Not a big issue, but I can look at fixing that.

   The $extra parameter is up for the consumer of the activeselect module to use however they want. I'm using it to allow the currently selected component to be re-selected after the activeselect module runs and clears out the select box and then updates it with the new components.

3. I based this on how the $string parameter is passed to this method. What I was passing for the $extra parameter changed a couple times before I settled on what's there now. This could be changed to use something other than '||' as the separator. I'll update that in another patch, along with the other bug above, soon.

I don't think activeselect will work very well here. We can't offer every possible option as a fallback, because then the select will be broken--it will offer invalid options if JS is disabled--options for other projects. Users would get validation errors and have no way of knowing which options were valid.

Maybe what we need is a selector to change projects. A button next to the project select that would be used to switch projects. We do something similar in a project block.

wow, yeah, i had no idea activeselect "fell back" to just presenting every possible option if JS is disabled/insufficent. that's no good at all. :(

i think i'm with nedjo on this one... if that's the best activeselect can do, i don't think that's the right approach here. i'm changing the title of this issue to reflect this, and setting the status back to active.

i'm not 100% sure how the UI should look/work for nedjo's proposed project-selector, where there's a separate button to just change the project. in a way, this will be something like the original issue submit when the project isn't already known, the dreaded 2-page form that was a constant source of trouble as the forms API kept changing in the lead-up to the 4.7.0 release. :) but, basically, if you click the "change project" button, you'll end up on "page 2" of the issue form with the new value for project filled in. might not be that hard to get this working, since so much of the plumbing is already in place for this... maybe the only UI change is to add this new button in the "Project information" fieldset and we're done. any volunteers for a patch? :)

any other proposals (besides activeselect or this "change project" button) that would be a better UI for this?

thanks,
-derek

bump: d.org infrastructure issue http://drupal.org/node/81805 is duplicate with this...

attached is a first js crack at this problem. .js file to follow...

.js file...

needs to go in the same folder as project_issue.module

missing an unserialize

dynamic form fields should be required.

first try at adding support to keep component and version selections.

.js file for that last change.

.js file

attempt to keep revision setting.

new .js file for above

nope, sorry, version still gets cleared when you change projects.

but, i should add... this is going to majorly kick ass when it's done, and this version reset thing is really a minor gripe in an otherwise very cool UI improvement. thanks so much for all the effort on this!

ahh, 1 more minor problem: on the initial issue add form, if you start with project foo (e.g. node/add/project_issue/foo) it'll have components and version from foo. if you change to project bar, nothing gets reset, and you're stuck with the choices from foo.

attached is a fully tested and working implementation of this feature. it disables the dynamically loading selects during the load, so the user can't continue until the new options are loaded, and it preserves the selected version and component information if it can. since category is required, we might want to eventually add that to this process, but i think we're fine for now. you'll need the new .js file i'll which i'll attach to the next followup.

this should degrade just fine into the original behavior with no js, but i have not tested. nothing should happen at all without js enabled, since the whole thing starts with a js event.

and the new .js file -- goes in the same location as the project_issue.module file...

i love it! works great for new issues and issue followups, for both previews and submits. saves the value if there's a match, resets to [none] if not. someone should test/verify what happens w/o JS, but it'd be hard for it to do worse than we already have. ;) this is 98% perfect...

however, the remaining 2% is probably my problem. ;) there's magic float/clear stuff going on in these fieldsets to keep the UI nice and compact (http://drupal.org/node/62129). something about the new <span> is screwing this up, and it's effectively clearing the float. :( perhaps i can get someone with some serious CSS-fu to help figure this out. also, i might try the new clearing class in 5.x and see if that behaves any differently/better (http://drupal.org/node/64292#clear).

thanks again for all the work on this hunmonk, this is great!

i don't think it's an issue with the spans. the theming looks perfectly fine in garland. check your CSS in the theme you're using. :)

and this won't be implemented in 4.7. reclassifying to HEAD.

after further inspection, this appears to be a safari problem. looks fine w/ ff (and opera, according to hunmonk) but is broken w/ safari. :( i still think the new clearing class might help. let's try that before we commit.

and yes, agreed on moving this issue to HEAD. ;)

this patch makes http://drupal.org/node/105227 much more likely to occur. so, this issue is hereby on hold until that one is fixed (which doesn't have to be long). there are patches in #105227 that need review (for both DRUPAL-4-7--2 and HEAD) so please review those if you're interested in this.

thanks,
-derek

series of screenshots to demo safari's lame handling of span + float/clear + inline-table.
this is the default project_issue.css + hunmonk's latest patch.

so, there's a <span> around both version and component, inside the "Project information" fieldset.
.project-issue .node-form .options fieldset {display: inline-table;} in project_issue.css

this is from modified project_issue.css + hunmonk's latest patch.

still the <span> around both version and component, inside the "Project information" fieldset.

.project-issue .node-form {display: inline-table;} in project_issue.css...

(this at least fixes the newline inside the fieldset, but it restricts the width of the body to the width of the widest fieldset).

no inline-table at all in project_issue.css. basically fine, except *every* fieldset is now full width.

the existing UI: no spans inside the fieldset, and the existing .css:
.project-issue .node-form .options fieldset {display: inline-table;}
fieldsets are only as wide as they need to be.

ok, http://drupal.org/node/105227 is basically RTBC, and seems to fix one of the main problems still in here.

furthermore, UnConeD was kind enough to help me clean up the JS to not use the <span> crap at all, which means all the float/clear/inline crap from the previous screenshots is now resolved. ;)

this is now nearly RTBC. i wouldn't mind another few reviews, but i'm quite happy with the current state of things.

thanks, everyone!
-derek

p.s. this single patch includes the new improved project_issue.js, no need to attach them separately.

some code cleanup:

1. made the jquery selectors more consistent/efficient
2. did some single/double quote swapping for consistency
3. wrapped the mod_project function in the jsEnabled test
4. changed $default = NULL to $default = 0 -- more accurate

ran some quick tests, and everything still works fine. the code is solid. let's get this sucker in... :)

committed a modified version to HEAD. i replaced tabs with spaces as appropriate, and added a few comments. otherwise, that was indeed RTBC. ;) now installed on s.d.o.

thanks!
-derek

Very welcome improvement, nicely done.

Two minor issues, 1. will the url passed by the onchange js break without clean urls? 2. for consistency with what we do elsewhere, should we avoid the hard-coded onchange and instead attach to the event dynamically on page load? Here's a quick untested (sorry, no HEAD project module test site at present, I'll test when I get one set up again) suggestion for how we could do this.

We pass the url as a 'setting' through drupal_add_js().

i will say that i've tested the current HEAD code on clean and unclean URL sites, and it works on both. so, what's in HEAD right now isn't currently broken in the way nedjo suspected it might be. however, i tested nedjo's patch on a local test site (unclean urls) and it still all seems to work great.

the logic for the patch seems sound, and the code appears reasonable, but i don't really understand the implications of the onchange stuff. i must admit i have basically *NO* js knowledge (yet), so i'll have to wait for hunmonk or unconed to give this the RTBC is most probably deserves. ;) since it seems to keep all the JS-related mojo in the .js code, not the FAPI code, i'm certainly happier, and since it still works, i'm giving this a huge +1. but, i'll defer to the experts...

thanks, nedjo! great to see you active in the issue queue again. ;)
-derek

+1 on the approach -- definitely cleaner. attached just corrects some minor spacing issues and adds a bit more clarification in the code notes.

tested as working on my local install

committed to HEAD. thanks, y'all!