the Version and Assigned select box data are both determined by project, and are loaded correctly when a page load first happens for either an initial issue, or a followup.

however, the project select can be changed, and especially on the issue page itself, which has no AJAX reload when the project is changed, wrong Version and Assigned data can be submitted, thus these fields need to be properly validated against the project that's been submitted in the form, not the project that was initially loaded. currently this is not working correctly.

CommentFileSizeAuthor
#5 pi_validate_fix.patch2.59 KBhunmonk
#2 pi_validate_fix.patch3.1 KBhunmonk

Comments

hunmonk’s picture

hunmonk commented
Status: Active » Postponed (maintainer needs more info)

ok, this is one ugly mess of a situation :(

for the moment, i'm only detailing what happens on the project issue submission itself -- i'll leave the followup stuff for another time. here's the fundamental issue we have:

first, these realities:

  • rid, component, and assigned all depend on their associated project.
  • forms are rebuilt upon submission, using the data available on the initial page load
  • the choice checker code occurs before any custom form validation

now, when somebody _changes_ the project then submits the form, the form builder code goes about it's business building the form elements based on the original project, not the posted project. we're posting a new project, but the rid, components, and assigned from the original project, and we're validating against the original project's form elements when we should be validating the new project's form elements.

fapi does nothing to help us discover the new project at the time of form building, so $_POST is the only option i see if we want to catch it that early.

now, we could forego the $_POST route and put custom validation into project_issue_validate(), but we run into a unique problem there: the form the user gets served after failed validation still contains the original project's rid, component, and assigned select boxes -- which means they'll never be able to get out of the validation problem without starting from scratch.

if we do go the $_POST route in form building, we get the added advantage of free validation from the choice checker -- because the built elements are from the new project and the posted value is from the old project, which the choice checker will catch. plus, of course, after the first validation attempt, the user now has the correct values in all the select boxes. "An illegal choice has been detected" isn't the most informative message, but at least the form is now set up for the user to succeed... ;)

adding jquery goodness to the initial issue form would eliminate a lot of this problem, except for anybody using the site w/o javascript.

so, given all of the above, how do we proceed here??

hunmonk’s picture

hunmonk commented
Version: 6.x-1.x-dev » 5.x-2.x-dev
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new pi_validate_fix.patch3.1 KB

aclight and i talked about this challenge today, and it occurred to us that a very simple solution might be to remove the option to select a project on the issue creation form.

here's why this seems like a good idea:

  1. completely eliminates all the validation related headaches for rid, component, assigned on initial issue submission.
  2. the user arrives at this form by either A) coming from node/add/project-issue, where they've already selected a project, or B) from a 'Create' link that is destined for node/add/project-issue/[uri], where they've also already basically selected a project.
  3. we block editing of metadata on the the original issue, so the project can't be edited there, either.

attached patch implements this approach. i ran some tests with both creation of the the main issue, editing of the main issue, and leaving followups, including changing the project. everything seems to work beautifully.

not sure i'm crazy about the display w/ this patch. we could implement it as a select box with just the one project as an option (either enabled or disabled), which would make the display more consistent, but may not be the best from a usability perspective. or we could throw some CSS magic at the form item to pretty it up a bit.

hunmonk’s picture

hunmonk commented

i've got the patch in #2 applied to project.drupal.org, and i'm feeling pretty happy w/ this solution. visually speaking, i actually think it's fine -- it just takes a bit to get used to it being different.

i believe this patch is ready to go in.

aclight’s picture

aclight commented
Status: Needs review » Needs work

I tested this out on p.d.o and I agree that it looks good as you've written it.

The patch looks good except for one minor thing:

       // Remove the 'Project information' and 'Issue information' fieldsets,
       // they're ugly after we move things inside the 'Edit issue settings' fieldset.
-      unset($form['project_info']['#type'], $form['project_info']['#title'], $form['issue_info']['#type'], $form['issue_info']['#title']);
+      unset($form['project_info']['#type'], $form['project_info']['#title'], $form['issue_info']['#type'], $form['issue_info']['#title'], $form['project_info']['project_display']);

I think it would be best do call unset($form['project_info']) in a separate line and add an additional code comment to make it clear why we're doing that.

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review
StatusFileSize
new pi_validate_fix.patch2.59 KB

how's this?

aclight’s picture

aclight commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. I tested on p.d.o and still seems to be working fine.

dww’s picture

dww
we/he/they
commented

I'll review/test this either later today or on Monday. I'd like to do that before it goes in. But, if I fail, and it's Tuesday without a reply from me, go ahead. ;) Thanks, folks.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Tested, reviewed, committed to HEAD + DRUPAL-5--2, and deployed on p.d.o and d.o. Thanks folks.

Status: Fixed » Closed (fixed)

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