IFAC edge case when metadata is edited directly without a comment

Heh. This happened to me recently at http://drupal.org/node/212375#comment-699188. I was *very* confused for a bit.

Shhh!! Don't get me in trouble with dww! Sheesh! ;)

Hm. Apart from suppressing the message unless the diff happened on /comment/ form submission (check form ID of referring form?), I don't really know.

Or, can't it diff to the current state of the node, rather than the last state of a comment?

I should probably stfu and read the code, but I'm juggling things atm.

i'd actually prefer to prevent editing of the metadata on issue node edit altogether, either via validation or hiding the form fields

Still, if it is the last comment being edited, it doesn't pose a problem. It would be great if a user can edit his own comment or issue, including metadata, as long as no follow-up is posted.

@leo_pape: this was rejected because it made the metadata diffing code more complicated and error prone. think of the metadata changes like a CVS commit -- you don't go hacking a CVS commit when you made a mistake, you make another commit to fix it. this 'only-forward' approach is also the best for the metadata, especially since, just like CVS, you can never be guaranteed that your 'last' comment is still the last comment, because others can comment to the issue.

this really needs to be fixed as i suggested above, so that initial issue metadata behaves like the current comment metadata upon edit.

Let's try this on for size.

Even better, with phpdoc

With project_issue_update() removed.

committed to 5.x-2.x. setting back to active until this is deployed on d.o et. all

Marking to critical due to a bug when a user who is not uid=1 attempts to edit the issue and is not allowed to.

This happens in the project_issue_issue_nodeapi() function in the following code:

      // Check if user has access, or if status is default status and therefore available to all,
      // or if user is original issue poster and poster is granted access.
      // If none of these is true, set error.
      if (!(user_access('set issue status '. str_replace("'", "", $state->name)) || ($node->sid == variable_get('project_issue_default_state', 1)) || ($state->author_has && ($user->uid == $node->uid)))) {
        form_set_error('sid', t('Invalid issue status %status: you do not have permission to set this status', array('%status' => $state)));
      }
      break;

This patch is even simpler than it looks. The first hunk is really just rewriting some conditional statements to not be so obtuse. The second hunk is just indenting the code into an if clause. I tested this as uid=1 and a site admin that can edit nodes and it works properly in both cases.

from the code comment for the status validation:

or if user is original issue poster and poster is granted access

do we need that check given that metadata fields are only displayed on new issue nodes?

we don't even need this validate case. it's cruft from pre-fapi days. the project_issue_status() function already limits the status dropdown to the statuses that the user can set, and fapi handles validation from there.

Code looks fine, and functionality works with admin and regular users and verified that the "author may set" stuff still works as it should as well.

Commited in #104539 and deployed on drupal.org and project.drupal.org.

Actually, it was http://drupal.org/cvs?commit=104696 where this was finally fixed.