Prevent cross posting from reverting metadata fields

The root of this problem is that since issue followups are now true comments, we've enabled the display of the comment form on the issue page itself, instead of requiring the user to hit reply first. Thus, the comment form can have "older" data on it if it sits in a users browser for some time before they submit their comment. This has always been a problem, but it's just more obvious now that comment forms are on the issue nodes themselves.

Other than going back to forcing users to hit reply to create a comment on an issue, or possibly by using javascript, I'm not sure how we could really fix this.

#281476: Prevent cross posting from reverting issue information has been marked as a duplicate of this issue.

Excellent! I knew there had to be an existing issue for this problem. Let me repeat what I said in #281476:

I envision a fix for this by using some hidden fields that hold the original values. That way the backend PHP can compare the original to the "new" value and if no changes are made, those fields won't be updated when posting the comment.

Also, when previewing a comment, any field that has no changes should be updated to reflect any changes that were made since the user first started typing the comment.

Side note: I think this already works correctly on preview -- if you preview, and someone else changed something, you'll see that you're about to change it back.

Anway, I think that comments should basically send a "diff" of the metadata when the author is intending to change something, instead of just sending a set of current values. This would be really helpful for mailhandler support and accepting issue followups via email, too.

With testing bot now automatically setting issues to CNW, this is going to come up more frequently, and will result in re-tests of known failed patches if it's not caught by humans.

This is not everything as there needs to be validation to avoid clashes and not all properties are covered but it already is a hell lot better. Most of the code is shuffling code around in issue_node_form.inc so I can reuse it. Also it would be shorter if I would know how to write that if as a terniary but I have no idea how to use references w terniary.

http://bugs.php.net/bug.php?id=12247 ?: is an expression so no references to make the patch smaller.

Edit: so far the patch fixes the crossposting of keys listed in _project_issue_node_form_values. I know title needs similar help. What else did i leave out?

+++ includes/comment.inc	19 Dec 2009 22:04:38 -0000
@@ -46,6 +46,22 @@ function project_issue_comment(&$arg, $o
+          if (in_array($key, array('category', 'priority', 'sid'))) {
+            $posted_value = &$arg[$key];
+          }
+          else {
+            $posted_value = &$arg['project_info'][$key];
+          }
+          if ($posted_value == $value && $posted_value != $old_data->$key) {
+            $posted_value = $old_data->$key;
+          }
+          unset($posted_value);

The preceding comment to this is nice, but I really think that the actual code requires some inline comments explaining what is being done here, too. ;)

+++ includes/issue_node_form.inc	19 Dec 2009 22:04:38 -0000
@@ -398,6 +367,41 @@ function _project_issue_form($node, $for
+function _project_issue_node_form_values($node, $type = 'value', $key = '#value') {

Missing PHPDoc. I think the default for $key should be #default_value, no?

$type doesn't seem to be passed differently, so superfluous?

This review is powered by Dreditor.

Re: "$type doesn't seem to be passed differently, so superfluous?"

Sure it is:

+  $form['original_issue']['project_info']['originals'] =  _project_issue_node_form_values($node, 'hidden', '#default_value');

vs.

+    $form['project_issue'] = _project_issue_node_form_values($node);

(using the default of 'value').

That said, chx, I'd like to know why we need 'hidden' at all. Why can't this just be 'value' in both cases? If it's actually needed (which I presume it is), it'd be good to have a code comment at the call site where we pick 'hidden' explaining why.

Also, agreed with sun on the inline comments for the project_issue_comment() code.

Sorry, been out all day and have a million things to catch up with, so no time for a closer review and testing, but this looks very promising!

Thanks!
-Derek

Added tons of comments. Still need guidance what I did not cover. Note that I might have missed something trivial because I do not know 100% what I have covered, I have merely reused what was there :)

Added title. I suspect we are done here because I counted 7 hiddens originally and added title and that's what I see in this issue: seven properties and a title. And I think I can even identify the *id ones: pid is surely the project, rid is likely the version, and sid is the status. The other four (category, component, assigned, category) are easily readable.

Yup, looks a lot better, thanks. Still a few typos etc, but I can fix those before I commit.

Re the 7 properties, yes, you correctly identified them all in comment #11. The only other thing that can be changed (at least on d.o) via a comment are issue tags. However, those are care of http://drupal.org/project/comment_alter_taxonomy. I'm not even sure off the top of my head how *it* records and handles its own data in the form. It's possible it already has some mechanism to handle this problem (although I doubt it). More likely, it just needs a completely separate patch in its own queue for this. Either way, I don't think it should hold up this patch, but I wanted to mention it in case chx is feeling inspired and wants to dig in there. ;)

For extra credit, you could open an issue in the http://drupal.org/project/comment_cck project about this same problem, since it's trying to do the same thing. For bonus extra credit, you could post a patch. ;) But, we don't (yet) use that on d.o, so it wouldn't be necessary in the short term.

Thanks again!
-Derek

Deployed this on d6.p.d.o for testing. Doesn't seem to handle rid (version) properly:

http://d6.project.drupal.org/node/218066#comment-2307283

Rerolled for the following:

A) The variable names were confusing, both before this patch, and the ones in the patch. Renamed $old_data to $current_values_pre_comment -- it's more verbose, but at least it documents itself. Renamed the ['project_info']['originals'] array added in the patch to ['project_info']['old_values_pre_comment'].

B) Added, clarified or otherwise fixed the code comments.

C) Fixed the bug I reported in #13. The code to handle when there's no $rid needs to happen after all the x-post prevention logic.

It's certainly better than what we've got now, but in testing edge cases, there are a few lingering bugs:

D) This doesn't handle the version well in the case of an issue moving projects. The original post works fine. A x-post first fails with "An illegal choice has been detected. Please contact the site administrator.". If you select something and save, you keep getting "You have to specify a valid version." and the version field fails validation, even though you have a real value selected. Only once you preview does the form work.

E) Speaking of previews... when you preview a cross post, it tells you you're going to change values you didn't mean to, and then when you post it anyway, the code in this patch prevents you from changing those values (see attached screenshots). Really, we should change the preview code to compare the current values in the comment with those saved in ['project_info']['old_values_pre_comment'] instead of doing what it does now (which is loading the current issue).

I don't really have time to keep working on this right now. I'm tempted to just commit the attached patch as a major improvement in the right direction, and worry about edge-case (D) later. However, the preview thing isn't really an edge-case, that's probably going to confuse a lot of people, so I think it'd be best to fix preview before this goes in...

I have factored out the function into a helper.

Cool, thanks! Getting closer, however:

F) Previewing still doesn't work on all attributes. See attached screenshots.

G) Although hook_comment() sucks and the API has $arg or $a1 or something stupid, we don't have to do the same in _project_issue_comment_avoid_crosspost(). There, $arg should be called $comment.

Subby.

hm, this is highly interesting and seems like a lot of improvement was already done, mucho gracias for that

point E in #14 kind of worries me as it could potentially cause a ton of confusion. but still, it's a major improvement

not sure if I'll be able to help much but I will certainly keep an eye on this and attempt to help out

I read through this patch, but haven't had time to test it yet. The one thing that stood out, (and maybe I just need to apply it and see what it's really doing) is that we seem to be special casing both preview and have some duplicated code between the two (project_issue_comment() and project_issue_form_comment_validate()). Maybe I'm not following the flow between project_issue_comment and project_issue_form_comment_validate but if that duplication could be eliminated and we check this once it would eliminate the errors with preview mentioned in #16.

BTW, the variable names are still as confusing as all hell. I'd propose moving $current_values_pre_comment to something like $current_values_pre_submit or something like $latest_values and old_values_pre_comment to just old_values, and treating the actual form submissions or $posted_data as chx has in _project_issue_comment_avoid_crosspost().

To clarify E) from #14, what should the default behavior on preview be? It seems that if the user hasn't changed any values, they should be previewed as the latest values, NOT what the user submitted (as long as they haven't been changed).

Yeah, the whole flow for comments is confusing and weird. D6 comment module didn't make this easy, and I don't think it's going to be much better in D7, either. :( Honestly, I don't remember the details that well, since hunmonk did most of the heavy lifting on this during the D6 port while I was busy doing all the views integration. But, if you can find ways to unify and simplify any of this, by all means, be my guest! Having a fresh pair of eyes come in and try to make it sane would be a very welcome effort!

However, there be dragons. ;) Be sure to read the code comments carefully. A lot of things seem stupid and weird because they are stupid and weird, but there wasn't any cleaner alternative (at least not that we were aware of when we were porting). Obviously, any big refactoring will require a lot of testing. Speaking of which, if you wanted to add any simpletests for any of this, you'd quickly become my new best friend. ;)

Re: variable names, yeah, I struggled with finding reasonable names for those variables. :( It's really damn hard to come up with good ones that are clear and precise, but not 95 chars long. ;) In IRC, we came up with:

[5:10pm] dww: mikey_p: AFAIK, there are 3 sets of values, right?
[5:10pm] dww: 1) the values in the form at the time the comment form was loaded
[5:10pm] dww: 2) the values submitted as part of the comment form
[5:10pm] dww: 3) the values of the issue just before the comment was posted and any potential changes are made
[5:10pm] mikey_p: dww: yeah i was thinking $old_data_pre_comment $current_data_pre_submit and $posted_data
[5:11pm] mikey_p: #3 was throwing me off
[5:11pm] dww: mikey_p: so, you're saying #3 should be "$current_data_pre_submit"?
[5:11pm] dww: mikey_p: what's it called now?
[5:12pm] mikey_p: $current_values_pre_comment
[5:12pm] dww: ahh, and that threw you off?
[5:12pm] dww: the current values of the issue per this comment being posted...
[5:12pm] dww: you want to emphasize "pre submit" vs. "pre comment"
[5:13pm] dww: so #3 isn't confused with #1...

Sure, I guess that makes sense. So, we'd have:

1) $old_data_pre_comment -- the values in the form at the time the comment form was loaded
2) $posted_data -- the values submitted as part of the comment form
3) $current_data_pre_submit -- the values of the issue just before the comment is posted and any potential changes are made

Re: #20: The preview is previewing the comment, not the issue. If you didn't try to change any values in your comment, your preview shouldn't show any changes. I hope that's clear. Please ping me if not.

Frankly, stepping back, I think this whole approach is more complicated than it should be. We're going down this path since this is the way chx started the patch based on trying to change as little as possible. I honestly believe it'd be vastly clearer if we approached it the way I suggested in #4:

Anway, I think that comments should basically send a "diff" of the metadata when the author is intending to change something, instead of just sending a set of current values. This would be really helpful for mailhandler support and accepting issue followups via email, too.

Although the form itself would still need a full set of form values as posted, and a full set of the values in the issue at the time the form was built, I think the APIs should all be structured to accept an array of "diffs", much like what project_issue_metadata_changes() returns. So, the form handler should just compare what was posted vs. what was in the issue at the time the comment form was built. It take the array of any changes and pass that array around to the underlying API functions that process the comment and potentially update the issue... It's been a while since I really looked closely at all of this, so it's possible that such a change would be even more confusing and difficult, but I have a sense it'd be a lot more clear that way. Something to keep an eye on as you open up this patient for surgery. ;)

Thanks!
-Derek

Well I reviewed the patch in #15, and I do have to say that it works as advertised and a cross post after updating from a separate account does not revert metadata. Yay!

However, what this is actually only one possible scenario with what amounts to a 3-way merge. I'd need to re-read the code from _project_issue_comment_avoid_crosspost() to see exactly how it handles each possible case. I've drawn a chart up here with what I think would work best, with the exception of a merge conflict. I'd think the easiest way to handle that conflict would be a validation error, with the new form values set to the current values from the DB.

As it is I think we could reuse some code from validation and inserting a comment.

@Pasqualle: that's exactly what the "Result is B" path through the graph gives you. "do not change the metadata" is equivalent to "use the metadata values currently saved with this issue, not what's in the form POST", which is what (b) is. ;)

It'd be nice to drive this home soon. If comment preview is hard to get right, we should just leave that to a follow-up.

Patch no longer applies, I tried to fix the failed hunks but now I'm getting a string of warnings when previewing. I'll try to work on this later.

Straight re-roll to keep up with HEAD.

Actually, I am not able to reproduce the comment preview issues with the latest patch. Method:

• Tab 1 and 2 load an issue
• Tab 1 submits a comment setting the issue from 'active' to "won't fix"
• Tab 2 previews a comment without changing any status, still sees "active" in the dropdown, but the meta table is absent.
• Tab 2 previews again with "needs work", meta table displays "won't fix" -> needs work".
• Tab 2 previews again with "active", meta table is absent (even though the issue is currently "won't fix").
• Tab 2 posts, issue is still at "won't fix". No cross post!

So the only thing that isn't in sync is the value of the dropdown, except it was a little odd that in 5, I wasn't able to set the issue back to "active" (an "intentional" cross-post). But in that situation, you could just post again with the intended status change, and no meta fields are *accidentally* reverted.

Based on that, setting this back to "needs review". Unless someone cross-posts :P

carlos8f: Want to take a stab at writing some tests for this? I know this is probably pushing simpletest to it's limits, but I think we should be able to take the first page and store it in separate variable and then do the intermediate request, restore the first page and attempt to x-post and change the metadata. I think that drupalSetContent()* and drupalSetContent() should be able to handle the content manipulation.

(although I had to hunt to find the correct getter/setter, -1 to unnecessary drupal prefix on method names)

@carlos8f: Right, the status field happens to be handled correctly in the preview case. That's actually visible in my screenshots from #15. However, try with version, component, or assigned, and you'll get much more confusing preview behavior. That's what I'm talking about needs work.

Also, I'd really *love* to see some tests for this patch. I'm not sure how to trigger a cross-post from inside simpletest, but if anyone can figure out how to do that and write a bunch of tests for this, I'd be extremely grateful! I'm not going to consider it a blocker for a commit, but it'd be great if we had tests for this. However, I really don't want to make preview worse than it is now.

Thanks,
-Derek

Here's the basic approach that I think would work for testing cross post, provided the form cache isn't screwed up by logging in and out. In pseudo-code:

// Have 2 created users, a project, and issue from the setup method

// Login first user
$this->drupalGet('path to issue');
$this->stale_page = $this->drupalGetContent();

// logout, and login ad 2nd user
$this->drupalGet('path to issue');
$this->drupalPost(null, $form_values);
// assert that the issue is updated

// logout, and login as first user

$this->drupalSetContent($this->stale_page);
$this->drupalPost(null, $other_form_values);
// assert cross post was prevented

Wouldn't it just be easier to add a hidden form field for node updated time. When comment is posted compare hidden value to current node. If the same just process. If different compare submitted status values with current node values. If they all match just process (while at it dont cause a node_save() and add a revision). If they don't match trigger a validation error and force user to preview and adjust their post.

Should be a cleaner patch and most likely if someone else changed the status updates even if the posted changes don't match it might be cause for a review of the comment.

Someone else should give this a try.

Apologies for hijacking this thread but I think it warrants some investigation in terms of the recent D7 port initiative

Yes, although the problem is going to be totally different (and in some ways, worse) in D7. See my last paragraph at #1545952-10: [META] UI for updating an issue in D7.

I'd rather keep this issue itself for D6 since that's what all this discussion and code is about, and deal with the related problem in D7 over at #1559578: D7 solution for when multiple users are updating an issue simultaneously.

Thanks,
-Derek