Now that we have a hook_project_issue_assignees(), we need to re-implement that hook every time the project is changed (eg. from a comment) so that the list of users in the assigned select box is also updated.

Attached patch does this.

Comments

hunmonk’s picture

Status: Needs review » Needs work
  1. the validation code for the assigned dropdown seems wrong. first, project_issue_assigned_choices() always returns an array, so the !isset($assigned_choices) will always pass. second, if we're trying to make sure that the submitted user is a valid assignee for the new project, won't the FAPI choice-checker take care of that?
  2. if the js code needs to know the issue nid (which is a static value from the perspective of the AJAX we're using here), then it seems more proper to pass it as a js setting instead of a hidden form value.
  3. // Build the HTML output for the rid select. -- this code comment looks wrong for building the assigned select
  4. function project_issue_update_project($pid, $cid = NULL, $rid = NULL, $assigned_uid = NULL, $issue_nid = NULL) -- the order of the args there seems goofy to me. i think it would be less confusing if we reorganized those instead of just tacking the new args on the end. perhaps pid, nid, cid, rid, uid? just some order that makes intuitive sense...
  5. the assigned select you're building via AJAX has it's #required value set to TRUE, but i'm not seeing where that's consistent with the way the form item is normally built -- is there a reason for that?
hunmonk’s picture

Status: Needs work » Needs review
StatusFileSize
new6.28 KB

attached patch:

  • rips out the validation code for the assigned user that was added in the previous patch. after talking w/ aclight, neither of us can determine why it's necessary
  • moves the issue nid to a js setting
  • reorders the args of project_issue_update_project() to something i think is more sensible.
  • fixes bad comment in #3, unneeded #required attribute mentioned in #5 above, and cleans up some more inconsistencies with the code that adds the assigned select box. all the errors look like copy/paste issues.
  • adds doxygen to project_issue_update_project()
  • fixes the node_load() for loading the issue in project_issue_update_project() to make sure it checks that the passed nid belongs to a project issue. also changed the node_load() for the project loading for consistency.
  • fixes the code that disables the selects during the AJAX call -- the extra arg in .attr() broke it.

tested, and this all seems to be working beautifully now. gonna leave it CNR for a bit in case anybody else wants to jab at it...

dww’s picture

Why are we doing this:

-  $node = node_load($pid);
+  $node = node_load(array('nid' => $pid, 'type' => 'project_project'));

in light of this: #311369: Project loading could use the static cache ?

Is the idea that the static cache doesn't help us in this AJAX menu handler, since we're only looking up a single project per "page load" anyway?

Otherwise, patch looks good visually. I haven't tested it yet.

hunmonk’s picture

no, the idea is that we have no real validation on the pid, we're just trusting the url that the AJAX request sends.

and also what you said about single page loads. :)

aclight’s picture

StatusFileSize
new6.68 KB

This patch fixes the doxygen of project_issue_update_project() to start with a one line summary and also fixes the @param assigned_uid part. In addition, removes a blank line from the start of the function.

aclight’s picture

StatusFileSize
new4.47 KB

with updated comment

aclight’s picture

StatusFileSize
new6.7 KB

unified diff this time

dww’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested. Please commit to DRUPAL-5--2 and HEAD. Thanks!

hunmonk’s picture

Status: Reviewed & tested by the community » Fixed

committed to 5.x-2.x, HEAD, deployed on d.o

Anonymous’s picture

Status: Fixed » Closed (fixed)

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