Problem

  • After posting an issue comment, the issue summary
    • shows the comment author as issue summary author.
    • creation date is changed to the time of the comment.

Status

Issue is postponed and will be fixed during Drupal.org D7 upgrade.

Details

  • Issue summaries are based on node revisions. Issue nodes effectively have been turned into wiki pages.
  • The expectation for updating issue summaries is:
    • the issue node author remains the same.
    • the issue node body shows the current node revision.
    • the author as well as the creation date of the issue summary (node revision) is output.
    • the author and creation date of the current summary revision may be different to the node's author and creation date.
  • However, project_issue allows to update the issue node from the comment form.
    • A comment may change the issue title. The existing node revision is updated when a comment is posted.
    • This updates the author and creation date info of the issue summary (node revision) to the current user posting the comment and the creation date of the comment.
    • node_save() itself updates {node}.changed, and the helper function _node_save_revision() changes the node revision author to the currently logged in user.
  • project_issue keeps track of issue property changes in {project_issue_comments} - including the issue title - which allows to revert issue properties when unpublishing or deleting a comment.
  • Issue queue views are based on {node_comment_statistics}, so not related to neither {node}.changed or {node_revisions}.timestamp. In other words: Not updating these columns doesn't change the behavior of issues going to the top of the queue after posing a comment.

Proposed solution

  • Don't make project_issue call node_save() when a comment is posted.
  • Only update the issue node title, if it actually changed, and make sure to clear database and static caches afterwards.

Original summary:

Problem

When posting an issue comment, project_issue_update_by_comment() calls node_save(), which does this asinine behaviour:

      _node_save_revision($node, $user->uid, 'vid');

This ends up attributing the latest node revision to the currently logged in user, destroying the original author. See http://drupal.org/node/1036132/revisions for example. The author of the revision on top will always match the author of the latest comment on the issue (currently set to xjm, node revision was actually authored by jhodgdon).

There's an issue to fix this upstream in node_save() at #1217190: Interaction between project issue's comment behavior and node_save is causing revision author to update to comment author; however, since that fix is likely a loonnnngg way off and d.o is actively losing revision authoring information now, it made sense to split this off into its own issue to see if it could be solved independently in PI module.

Proposed resolution

At #1217190-4: Interaction between project issue's comment behavior and node_save is causing revision author to update to comment author, mikey_p suggests the following options:

1) mocking global $user (also oddly backed up by chx, as long as we wrap it like so to prevent accidental account hijacking:

session_save_session(FALSE); $user = user_load($node->uid); node_save($node); session_save_session(TRUE);

2) mocking node_save() or something like it that can save the revision (if revision settings are set that way) or update the revision and node tables as needed, without overwriting node data with global vars. This would also require mocking all the nodeapi hooks, which is one of my bigger concerns as project* tends to rely on these for quite a bit of it's processing, and without them it's hard to tell what might break.

There's also:

3) (suggested by msonnabaum on IRC given his experience with node_save() and drush) Store the original $node->revision_uid and then do a direct UPDATE query to node_revision.uid to set it back after the fact when node_save() is done. Would break people using tools like Materialized Views to do data denormalization (wouldn't affect d.o, it appears, however).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

One other option would be actually saving a new revision, although project issue followups are one case this might not actually be desirable at all.

Damien Tournoud’s picture

I agree with catch here: if something changed in the issue it does make total sense to create a new revision.

That said, I'm not sure that every table in the project issue module is joined to the vid column.

Damien Tournoud’s picture

Another way of seeing it is that maybe node_save() is not necessary here, and we could just go away with calling hook_nodeapi($op = 'update') manually to signal that something has changed on the node.

sun’s picture

Status: Active » Needs review
FileSize
1.18 KB

Attached patch implements @chx's proposal.

webchick’s picture

Status: Needs review » Needs work

Actually, it doesn't look like this is enough. The frigging *create date* of the revision is getting overridden, too (see above). FFS. node_save()--;

#3 sounds like a good thing to try.

sun’s picture

Status: Needs work » Needs review
FileSize
1.6 KB

Simsalabim, no idea what I'm doing.

chx’s picture

If the problem is isolated to the node_revisions table then you can create a temporary table called uniqid(). 'node_revisions', set the prefix of the node_revisions table to that random string and call node_save(). If this blows up, maybe copy the node revision record into the false table. Update THAT.

webchick’s picture

Uh. What?

webchick’s picture

Status: Needs review » Needs work

sun's patch is deployed to http://issue-summaries-drupal.redesign.devdrupal.org/node/1149912 (user/pass: bananas) for testing.

It partially solves the problem, as you can see from http://issue-summaries-drupal.redesign.devdrupal.org/node/1149912. The revision is improperly assigned to catch, who was the last person to post a comment. But when Dries posts a comment later, it doesn't change it to Dries instead. Yay!

However, it does change the create date of the revision to the same timestamp as the comment (july 14 vs. june 16), and that's wrong. So needs work still.

sun’s picture

However, it does change the create date of the revision to the same timestamp as the comment (july 14 vs. june 16), and that's wrong.

+++ b/includes/comment.inc
@@ -574,9 +574,39 @@ function project_issue_update_by_comment($comment_data, $op) {
+  db_query("UPDATE {node} SET title = '%s', changed = %d WHERE nid = %d", array(
+    $node->title,
+    $node->changed,
+    $node->nid,
+  ));

As visible in the patch, we are updating {node}.changed only and we do not touch {node_revisions} at all.

Looks like the issue summary feature reads the changed date from the wrong table?

(On a related note: That changed date also seems to be output in the wrong timezone.)

EvanDonovan’s picture

Subscribing. Anything else to test here? - this was confusing me to no end the other day.

dmitrig01’s picture

chx is advocating something like

global $db_prefix;
$db_prefix['node_revisions'] = uniqid();
node_save($node);
unset($db_prefix['node_revisions']);
sun’s picture

I wanted to post this follow-up patch to fix the remaining issue, but...

The solution is wrong. With the patch in #6, we're no longer creating a new node revision when posting a comment.

This means that if you unpublish or delete a spam comment, there is no longer a node revision that could be restored. So the issue title won't be reset to the original/previous one, and I bet that pi will still restore the entire previous node revision, so I bet that we're actually losing/destroying data due to the patch in #6, which no longer creates the new node revision as pi expects.

For the very same reason, making the {node_revisions} update query hit /dev/null won't fix anything either.

Short of: As already mentioned in IRC, we need a proper summary of what is actually going wrong here, before attempting to fix anything.

sun’s picture

Title: Work around node_save() being a flaming POS » Posting a comment changes too many issue node revision properties
Status: Needs work » Needs review
FileSize
6.29 KB

Attached patch implements option 1) of #13.

Behavior documented inline. Did not check the @todo yet.

sun’s picture

Sorry, forgot to remove the change to the node revision uid.

sun’s picture

Project: Project issue tracking » Drupal.org customizations
Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Project: Drupal.org customizations » Project issue tracking

sun: We were never creating a *new* node revision when saving a comment. We were editing an *existing* node revision.
http://drupal.org/node/1217190#comment-4739798

sun’s picture

Quoting @mikey_p:

Project issues does keep track of all of the properties that are changed and it keeps track of the places the changes were made either in project_issue_comments or in the comments table. Project issue isn't really at fault here, since its arguable that even if the title *wasn't* changed, that you'd still want to update the node so that it would have an accurate time stamp for an issue.

I doubt that issue queue views are using {node_revisions}.timestamp though. They're likely based on {node_comment_statistics}.

I thought I looked into {project_issue_comments} already, but now that I looked into it again, there's indeed a .title column in there.

In turn, the only update being required in project_issue_update_by_comment() should be to change {node}.title (perhaps also {node_revisions.title}?) and properly clear database and static caches afterwards.

sun’s picture

Project: Project issue tracking » Drupal.org customizations
Issue summary: View changes

Updated issue summary.

sun’s picture

Project: Drupal.org customizations » Project issue tracking
FileSize
3.13 KB

Updated the issue summary after discussing this once more with @webchick in IRC.

Corresponding patch attached.

Damien Tournoud’s picture

Status: Needs review » Needs work

If we are changing the title, IMO we should create a new revision too. I don't see anything wrong with that.

It's just a question of expectations around what is the "author of the revision". Angie seems to be expecting that it is the last person *changing* the body. Core is expecting that it is the last person changing anything on a revision. Both are just perfectly valid, but they are different.

It partially solves the problem, as you can see from [...]. The revision is improperly assigned to catch, who was the last person to post a comment. But when Dries posts a comment later, it doesn't change it to Dries instead. Yay! However, it does change the create date of the revision to the same timestamp as the comment (july 14 vs. june 16), and that's wrong. So needs work still.

We just cannot use the last revision timestamp and author for that. It's just *not* what they are meant for. Let's just add two new fields (summary_last_edit and summary_last_author) and update those fields when the *body* is being touched:

function drupalorg_issue_summary_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
  if ($op == 'presave' && isset($node->nid) && $node->type == 'project_issue') {
    // Load the old revision.
    $original_node = node_load($node->nid, NULL, TRUE);
    if (isset($node->body) && $original_node->body != $node->body) {
      $node->field_summary_last_edit[0]['value'] = time();
      $node->field_summary_last_author[0]['value'] = $GLOBALS['user']->uid;
    }
  }
}

As a side note: I don't believe core is wrong in defining the revision author and update date as it does. But it is certainly wrong in *actively preventing* us to change this behavior.

apaderno’s picture

The fields in the issue settings change the status of a issue: When I change the title, the component, the user to whom the issue is assigned, or the issue category I am not changing something for the comment I am adding, but I am changing the overall properties of an issue. Vice versa, when I am simply adding a comment, without to touch any of those fields, I am not changing anything in the issue's properties.
In the first case, the issue summary should be update, but in the latter case the issue summary should not change.

sun’s picture

Project: Project issue tracking » Drupal.org customizations
Version: 6.x-1.x-dev » 6.x-3.x-dev
Component: Comments » Code

We did not write a new node revision when the node title changed through a comment for many years, so I don't really understand why we suddenly want to start inserting node revisions in that case. The change is tracked in {project_issue_comments} already and sufficiently announced in the visible issue comment property changes.

Aside from that, I agree with @Damien Tournoud's proposal in #19, since the patches here are only touching project_issue code, and do not (and cannot) prevent whatever other code from calling node_save() for an issue node. Any kind of node_save() hi-jacks the issue summary data.

Thus, moving to drupalorg module.

Note that the patch in #13 contains a fix for the issue summary revision diff link.

ccardea’s picture

I thought this had been fixed, so I just want to report that it isn't fixed, in case someone else is also under that false impression. See http://drupal.org/node/1219592#comment-4754700. I just posted the first comment in this isssue, and I am listed as the author of revision 1.

ccardea’s picture

Just an offhand thought, but since we now have issue summaries where we can change the body of the issue, wouldn't is make sense to also move the title change mechanism to the revision process. That way you only need to call node save when there's an actual revision to the node, and you can get rid of this weird mechanism of changing the node title through a comment.

ccardea’s picture

Here's another offhand thought (sorry). Maybe a project issue needs to be an entity.

webchick’s picture

Separate issue if you'd like to discuss that, please.

EvanDonovan’s picture

@ccardea: d.o. still is running 6.x, it looks like from the Version number here and from the patches posted. So that would not be a solution till down the road a bit.

Also, it seems like #19 would be a good solution. I guess I didn't realize CCK was being used on d.o. now.

ccardea’s picture

@EvanDonovan

I know. That doesn't say much for Drupal 7 but, this is not the place to discuss that. Call me a dreamer. I posted an issue here. http://drupal.org/node/1224438

rfay’s picture

Subscribing. Thanks for the great work on this.

merlinofchaos’s picture

I would just like to point out that 'fixing' this is not really fixing much of anything.

Project issue is basically a long revision system using comments to track changes.

Trying to merge this with the existing node revision system creates two separate revisioning systems that cannot communicate with each other.

Example 1: I edit an issue and change the title. The following problems occur:
1) No notification is sent (project issue does that when comments are changed)
2) The title change does not show up in the issue comments, so tracking of that change appears lost. It's not lost, but it's recorded in a different system.

We really need to try and move to using the project issue revisioning system so that we can keep clean trails and clear notifications when changes are made.

pwolanin’s picture

+1 for #19

ccardea’s picture

I apologize for my comment in #22. It was based on:

It partially solves the problem, as you can see from [...]. The revision is improperly assigned to catch, who was the last person to post a comment. But when Dries posts a comment later, it doesn't change it to Dries instead. Yay! However, it does change the create date of the revision to the same timestamp as the comment (july 14 vs. june 16), and that's wrong. So needs work still.

I kind of forgot that was on a dev site. If I had read through the rest of the issue, I might have gotten a clue. I still think that Project Issue Tracker needs to be retooled. I'd be interested to know if anybody's working on that. Sorry I can't offer more help solving the immediate problem.

webchick’s picture

I don't really understand #29. You can't change the issue title from the edit tab, only via a comment. This is by design.

ccardea’s picture

Theoretically, I think #29 is right. At present, Project Issue is using the comment system to mange the attributes of the issue. In order to use a single revision system, I think the issue summary would also have to be treated as an attribute of the issue.

webchick’s picture

#29 is suggesting a feature, which is "make the issue body editable from a comment." That's a fine feature request, and can go in another issue.

This issue is about fixing a bug, whereby project_issue is obliterating node revision data when comments are posted. This bug should be fixed, regardless of how the issue body is updated.

jhodgdon’s picture

RE #30 - the idea in #19 does not actually solve the problem. The revisions tab would still display the wrong information, since it would still be taken from the info in the node revisions table.

sun’s picture

The revisions tab would still display the wrong information, since it would still be taken from the info in the node revisions table.

That is a hard-coded behavior in Drupal core, which could be understood as a "bug", but won't change anytime soon. It was the topic of the corresponding core issue #1217190: Interaction between project issue's comment behavior and node_save is causing revision author to update to comment author, which I've closed, since even a stop-gap fix would require API changes, and thus would be close to impossible, and in turn, the behavior should rather be changed in a more carefully planned initiative to revamp revisions.

In short: Any code that calls node_save() will trigger this behavior and there is nothing we can do to prevent it, since we can't change all node_save() calls. Unless you want to do a stop-gap fix for aforementioned core issue for D8 and wait 1-2 years to get it backported to D6, the only thing we can do here is to forget about usernames and changed dates in node revision data.

Hence, we only have two options:

  1. #19: Add hidden CCK fields to project_issue nodes: summary_author and summary_changed.
  2. #29: Re-implement issue summaries to be based on project_issue's existing architecture that allows comments to update the issue node, and specifically record and track the summary body and username in the designated {project_issue_comments} table (i.e., identically to all other issue properties).
jhodgdon’s picture

No, we also have two more options, although both of them are kind of hack-like:

3) Don't call node_save() in project issue when a comment is being added. Instead, use a custom save function that doesn't update the revision information.

4) Go ahead and call node_save(), but because we know it's going to overwrite the revision information, overwrite global $user just before so that it doesn't put the wrong information in there.

That said, #2 probably makes the most sense, but it's definitely the hardest. #1, as I said before, doesn't really fix the whole problem.

seanberto’s picture

.

ccardea’s picture

@ jhodgdon Isn't option 3 what sun did in his patches? What's wrong with the patch in #18? It looks like the objection was maybe we should create a new revision when the title changes. Can we agree that for this purpose we don't want a new revision just for a change of title? We only want the revision author and date to change when the issue summary changes.

I'd like to point out that this was partially fixed as far back as #6, with the only remaining issue being that the timestamp of the revision was somehow changed to the date of the comment. If #18 doesn't solve that, then the problem is coming from somewhere else.

ccardea’s picture

I suggest that patch#18 should be tested. If the problem with the revision date persists, try running it without calling the node hooks. If the problem goes away then you know there's some other module updating the revision table.

ccardea’s picture

Just looked at the commit diff that created the mechanism for project issue summaries. It looks like the date on the issue summary is coming from node->changed, so updates to the revision table should not affect it.

ccardea’s picture

Status: Needs work » Needs review
killes@www.drop.org’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed the patch and I am comfortable with deploying it.

mikey_p’s picture

Status: Reviewed & tested by the community » Needs work

From a project* maintainers perspective, the approach in #18 isn't acceptable. This is a significant change in the behavior of project module, to accommodate the needs of a feature that exists solely in drupalorg customizations. I am in favor of damien's proposed solution in #19 and also the 2nd option from sun in #36, in that order. This will allow that fixing of the project summary display information that has been incorrect since the feature was deployed, which we seem unwilling to disable or hide in the meantime.

Any option not calling node_save as it is in project at the moment stands the change of breaking something else that could be relying on the node revision being updated as it currently is when commenting on an issue. Mocking $user isn't going to work either, as it won't prevent the overwrite of the timestamp on the revision, not to mention, mocking a global is rather wrong, and a bit of a security nightmare to even consider.

killes@www.drop.org’s picture

I am not partial about how we solve that. I however want to see it resolved soon. This feature has been hailed as important and a life saver, but now it looks quite broken, this is annoying.

ccardea’s picture

@ #44 Project and project issue don't really have much use outside of Drupal.org, but they are extremely important to Drupal.org, so I think that it makes more sense to cater to the needs of your biggest 'customer', rather than to some hypothetical other customers. It seems to me that the change wouldn't necessarily have to be propagated outside of Drupal.org. The only thing that gets updated by a node revision is the title, and that still happens if the the title changes.

One of the reasons this needs to be tested is to find out if indeed it breaks anything else. But it shouldn't, because the node hooks are still being called. Unfortunately my own attempts to acquire an environment where I could test this have been thwarted, so it looks like testing will be up to someone else. If you favor a different approach, how about supplying a patch, instead of just blocking this from being fixed.

Who gets to make a decision on this?

ccardea’s picture

@ #44 I don't think option 2 in #36 is practical at all with the current architecture.

sun’s picture

Strange follow-ups here: I'm the author of the current patch, and I clearly stated that the patch is bogus and won't resolve the issue. But nevertheless, people are suggesting to commit that patch. Um?

Additionally, two alternative solution approaches that will work have been outlined already. But instead of implementing one of them, people are bashing on @mikey_p for rightfully rejecting a patch that wasn't approved, and are asking him (a project_issue maintainer) to fix a bug in a completely different project (drupalorg module). Um?

To move forward here, one of the solutions outlined in #36 (1. maps to #19) needs to be implemented.

ccardea’s picture

When you say that the patch is bogus and won't resolve the issue, I assume you are referring to:

since the patches here are only touching project_issue code, and do not (and cannot) prevent whatever other code from calling node_save() for an issue node. Any kind of node_save() hi-jacks the issue summary data.

Correct me if I'm wrong about that, but under what circumstances would any other code call node_save() on an issue node?
a. When the node is edited. In that case, it would be correct to have a new revision and revision author.
b. Through a node_hook that gets invoked through the update_by_comment function.

What I'm suggesting is to test the patch and see if case b is occurring, because at this point we're still not sure what's causing the problem.

Also I realize that it's wrong to suggest that mikey_p supply a patch for a different project, but I left that comment in because it was Damien's idea, and Damien has not supplied a patch.

ccardea’s picture

When you say that the patch is bogus and won't resolve the issue, I assume you are referring to:

since the patches here are only touching project_issue code, and do not (and cannot) prevent whatever other code from calling node_save() for an issue node. Any kind of node_save() hi-jacks the issue summary data.

Correct me if I'm wrong about that, but under what circumstances would any other code call node_save() on an issue node?
a. When the node is edited. In that case, it would be correct to have a new revision and revision author.
b. Through a node_hook that gets invoked through the update_by_comment function.

What I'm suggesting is to test the patch and see if case b is occurring, because at this point we're still not sure what's causing the problem.

Also I realize that it's wrong to suggest that mikey_p supply a patch for a different project, but I left that comment in because it was Damien's idea, and Damien has not supplied a patch.

ccardea’s picture

Sorry about the double posting. Actually it looks like Damien did supply some code in #19, but the patch would also have to remove the existing code.

ccardea’s picture

FileSize
140.16 KB
152.11 KB

OK, I managed to set up a local testing environment and applied patch#18. I did some minimal testing and the patch does appear to work. So far I:

1. Created a comment on an existing issue - no effect on on Revision author or date.
2. Created a new issue - Revision text appears fine.
3. Created two comments on the new issue - no effect on the revision text.

Unfortunately in this environment I don't have the capability to edit the Issue Summary, so I can't test that. Also the revision text is supposed to have a link to the previous version that didn't appear. I might need to do something to get this to work. If anybody knows please tell me.

I've attached a couple of screenshots, one taken right after the initial creation of the issue, and one after I posted the first comment. The issue was created by user auth2 and the comment was posted by user auth1.

ccardea’s picture

Actually there is only one revision, so it's correct that the link didn't appear. I'll test that if I can figure out how to turn on the issue summary edit capability.

webchick’s picture

ccardea: Cool, thanks for testing the patch. If you want to enable issue summary editing locally, just assign the "edit any project issue" permission to authenticated users. (A patch to the drupal.org testing profile to do that automatically as part of the installation would be welcome, as well!)

However, it looks like the patch in #18, even if it works, isn't going to fly from the project module maintainers, for reasons outlined in #36, #44, and #48. So we need to implement an alternate approach.

ccardea’s picture

@webchick Thank you.

Any thoughts on how this should be implemented? I don't want to spend a lot of time on this if its not what people want. The original suggestion was for CCK fields. If my local testing environment is correct, CCK is not enabled for drupalorg so that's not an option. That means we would have to add columns to a table, or create a new table. I'm thinking the 'project_issues' table would be appropriate. Also there's a question of what module this should be implemented in. I think drupalorg_project would be best, or drupalorg.

Damien Tournoud’s picture

@ccardea: CCK is actually enabled on drupal.org. Feel free to rely on that.

webchick’s picture

Drupal.org actually does have CCK enabled these days. Looks like that's another patch we need on the drupalorg_testing profile. :)

However, this problem is going to bite anyone who is using Project issue and the "edit any project issue" permissions. It's just more obvious on drupal.org because we have added that custom "Last updated on XX by YYY" thing that exposes the node_revisions table data right at the top of the node. So I do think it makes sense to fix it in Project Issue module, and not add extra dependencies like CCK.

So probably the right way to fix this, in addition to fixing an annoyance for email subscribers, is to do the plan outlined at http://drupal.org/node/1217286#comment-4784056. There's already an issue for this at #1230258: Make a comment for each wiki modification.

joachim’s picture

> #29: Re-implement issue summaries to be based on project_issue's existing architecture that allows comments to update the issue node, and specifically record and track the summary body and username in the designated {project_issue_comments} table (i.e., identically to all other issue properties).

Does that correspond to #1221190: The Issue summary should augment the original post, not replace it? If so, I think that's the right way to go.

ccardea’s picture

dww’s picture

I'm finally back online after nearly a month, catching up on all the issues and fallout that accumulated while I was gone. Not sure if I should post this here, at #1217646: Tweak UI of issue summaries, or if we need a more clear "Fix issue summaries" meta issue to coordinate all this stuff. ;)

While I haven't had time to fully ponder everything here, I lean strongly towards approach #19 from DamZ. Furthermore, we should just add a 3rd CCK field for the summary itself. #1223538-5: Make the new Issue Summary optional per issues queue is a pretty coherent UI proposal for how the summary can basically be optional on a per-issue basis (even if the proposal for opt-out per-project is IMHO won't fix). Issues where the summary field is blank will get a local action called "Add summary" and otherwise, none of the existing UI about the summary. Once there's a summary, the summary UI and edit tab appears, and the "Add summary" link disappears. See salvis's comment for the whole picture. Once the node exists, we'll form_alter body to have '#access' = FALSE so the OP can't be edited, only the summary. The field_summary_last_* ones will also be hidden in the UI and only set via a variant of #19. This would also enforce that the OP is never lost, and address the concerns at #1222082: Move Issue Summary text elsewhere.

The node_save() on comments will still make the revisions tab slightly confusing, since you'll still see the last user to comment as the owner of the current node revision. We'd have to somehow inject the info from the new CCK fields into the revision tab and diff, or do the nasty where we set the revision author back to the last person to edit the summary after each comment clobbers that, or just allow this confusing thing to continue in some of the edges of the UI. I am really torn on the proper behavior of updating the node revision when the comments are posted. Maybe there should be something that detects if the comment does change the issue meta data at all, and if so, reallly make a new revision, not just update the current one. Could be too much revision churn and clutter. A whole new node revision seems very expensive most of the time. I'm open to suggestions.

At least if we had these 3 new fields for the issue nodes, we could improve a lot of things about how the summaries work, even if it won't be perfect.

dww’s picture

p.s. Off topic, but I can't let it go without a reply:

@ccardea re: #46:

Project and project issue don't really have much use outside of Drupal.org, but they are extremely important to Drupal.org, so I think that it makes more sense to cater to the needs of your biggest 'customer', rather than to some hypothetical other customers.

Argh. We've spent *years* of hard work trying to undo this self-fulfilling and harmful assumption. It's *finally* starting to really pay off, and there's a wider pool of people contributing to the code than at any time in the 5 years I've been around. We're not going back down this road. If we hack Project* back into a place that it only works on drupal.org, we've got a huge support burden for the volunteers that maintain and upgrade drupal.org which absolutely no one else cares about or tries to improve. No thanks.

Otherwise, thanks for all your work recently on trying to improve things!

salvis’s picture

Thank you for picking up some of the pieces from #1223538-5: Make the new Issue Summary optional per issues queue.

From what I've read on this subject, it may be easier to support editing the node body (revisions, etc.) than a text field. That's why I suggested moving the original body content to a new field_original_body field (where it will be read-only), so that the node body can then be used for the summary.

If we want to push a template for summaries, then pre-initializing the freed-up body with the template would be a considerable usability improvement over having to hunt for it and copy/pasting it.

There may be an issue with input formats: the original poster may have used an input format that is not available to the person wishing to edit the summary. We may need to record two different input formats per issue node.

Where I suggested to have "Add summary"/"Edit" local actions (using the normal node edit tab), it would be worthwhile to spend some effort to have "Add summary"/"Edit summary" (customizing "Edit" in the with-summary state of the issue node), to get a consistent UX.

dww’s picture

Back on topic...

Although in principle I like #29/#36.2, and at one point I think there was even code in project_issue to talk to CCK and if there were CCK fields on the issue node those could be edited via the comment form, I think in the case of a text area, that UI would suck. I think the edit tab is a better place to edit a big text area summary, not a comment form. And I'd want to be able to use diff when the summary changes, not just print outs of both text areas (although I think there's a hook for modules to decide what goes into the comment diff table, and that could potentially change it into a link to the diff if we had separate node revisions to diff between whenever the summary changes).

OTOH, it's *really nice* if there's 1 consistent way to update things about an issue. I suppose we could solve the UI problems of trying to handle both a comment body and the issue summary text areas in the same comment UI. e.g. we'd hide the issue summary in a collapsed fieldset or something.

And the more I think about all this mess, the more I think project_issue should only call node_save() if it's really changing something about the issue, and in that case, it should actually be creating a new revision. I think. ;) I'm still undecided, but I'm leaning that way.

Sorry I'm not being more of a razor in the confusion here and proclaiming The Way It Should Be Done(tm) like I usually do. ;) I guess I still need to ponder this more and perhaps hash it out in IRC or something...

ccardea’s picture

@dww

RE: #61 I checked again and the Project* modules have quite a bit more usage than I thought. I'm not sure what I saw that made me think usage was something like 17, but I have to admit that isn't the case.

And the more I think about all this mess, the more I think project_issue should only call node_save() if it's really changing something about the issue, and in that case, it should actually be creating a new revision. I think. ;)

I fully agree that project_issue should not touch the node unless it's really changing something, but I don't think it should call node save at all when the title is changed through a comment, because it has the unwanted side effect of creating a new revision when that's not the intended action. I'm also against using fields to store summary author and date, because it duplicates information that's already available in the database.

Salvis's ideas sound good, but personally I don't get why all of this effort is being put into re-designing new features for a Drupal 6 module that will be unsupported in another two years or so. The current implementation was intended as a stop-gap solution, and it works well enough, if we could just fix this little bug.

webchick’s picture

I've not caught up on all the other replies in this issue, but just a note that whatever fix we employ here, this same issue is going to bite everyone that uses the "edit * project issue" permission in Project issue tracking module. The problem is just more obvious on d.o because we expose the most recent revision information on the node with some custom theming. But the node revisions tab shows the same behaviour, and that's coming from core.

So while we can fix this for d.o with a custom thing based off a CCK field, that doesn't help others who want wiki-style issues from Project issue tracking module. Maybe dww/mikey_p is fine with that, but just wanted to point that out.

joachim’s picture

> I fully agree that project_issue should not touch the node unless it's really changing something, but I don't think it should call node save at all when the title is changed through a comment, because it has the unwanted side effect of creating a new revision when that's not the intended action.

Maybe I'm missing something, but you can call node_save() and elect to *not* create a new revision, surely?

ccardea’s picture

Good catch, Joachim. Allow me to re-phrase. I don't think project issue should call node_save() at all when the title is changed through a comment, because it creates unwanted side effects. This was discussed at length earlier. We can get rid of the side effects by either not calling node save, or by storing and retrieving the data in CCK fields. Using CCK fields is redundant, harder to implement, and only solves the problem on Drupal.org.

For the sake of discussion, let's just think a little bit about what would be required to implement CCK fields.
1. The fields would probably have to be created in code, which would mean writing an update hook.
2. Since the fields would be hidden, there would be another hook needed to store the data in the field.
3. The existing code would have to be converted to retrieve the data from the fields.

I think it would be more efficient just to avoid the problem by bypassing node_save(). It also solves the problem for everybody, not just Drupal.org.

sun’s picture

node_save() has to be called to change the title of an issue. Otherwise, you'd never see the changed issue title, neither on a node page, nor in queues.

jhodgdon’s picture

To clarify: node_save() itself doesn't have to be called to change the title. What we would want to do is to do everything node_save() does except not update the revision author/date information, which is the update that is the subject of this issue. So it would involve making a custom not_quite_node_save() function that duplicates node_save() except for that bit of it.

joachim’s picture

> What we would want to do is to do everything node_save() does except not update the revision author/date information, which is the update that is the subject of this issue

http://api.drupal.org/api/drupal/modules--node--node.module/function/nod...

Unset $node->revision prior to saving it and the existing revision is updated rather a new one being saved.

As for the author problem, the current user object can be spoofed prior to calling node_save and unspoofed afterwards.

Though really, if node_save() is this inflexible, let's call it a bug in core and fix it.

ccardea’s picture

RE CCK fields: Did I mention the massive task of copying the existing data into the new fields?

jhodgdon’s picture

RE #70: there is zero chance of changing node_save()'s behavior in D6 at this point. So "calling it a bug in core and fixing it" is not a viable strategy of fixing the problem on d.o.

And you have not understood the problem, I think. The problem is that the node revision *is* updated with new author/date information. We don't want the revision to be updated, and we don't want a new revision with the comment's author and current date to be created, really. Ideally, we want the revision information to be left as it was, so that the person who last actually edited the node body is recorded as the most recent revision author, and the date the body was edited is recorded as the updated date. It is not possible to do that if node_save() is called, as far as I can tell. Right?

dww’s picture

Except that we *do* want the revision information updated when you're actually updating the node, which is what happens some of the time when you comment on an issue.

The real problem is that we want contradictory things. We want to display node revisions as if they're only possible if you're editing the issue summary, but we want to actually update the node via other means, too. It all comes back to what Earl said (I think earlier in this thread) about how project_issue already has a whole revision system, and we're trying to use a 2nd system for this one field, and it's causing us a big headache.

Which is why I'm saying that the extra CCK fields for keeping the summary as a separate thing (and record the last people to actually touch that) gives everyone what they want (sort of). Except it's a pain in the ass to do it all now. But "we" were all so anxious to deploy *something* to allow editable issue summaries that this whole thing was deployed without having encountered the problems we're now trying to solve. Which is fine, since even the sort of broken system we have now is better than nothing.

My first choice would actually be to just disable the confusing theme stuff on d.o that's spewing all this bogus information until we have accurate information to be displaying. Yes, it'd be *nice* to see when the issue summary was last edited, but that's not what we're displaying, so IMHO that theme stuff is a net loss (until we have real data).

project_issue was not designed to allow wiki-style editing. We had to fix it to even make that possible. So, no one else has this problem (yet). Which is why I'm okay with saying on d.o, since we're trying this non-standard configuration, it's okay to do something different and special (the extra CCK fields). So, I still lean towards that solution. It's not 100% ideal, but IMHO nothing will be 100% ideal. At least, no one has proposed anything so far that's 100% ideal (if they had, we would already be implementing it).

joachim’s picture

Ok, now I get it -- thanks for the explanation :)

The CCK fields could be added to project_issue as a dependency rather than our custom configuration. It would be a big change, but not a terrible thing. Other contrib modules do it, eg Station (and install code here: http://drupalcode.org/project/station.git/blob_plain/refs/heads/master:/...)

Another alternative would be for project_issue to handle the issue summary storage itself and add an 'edit summary' tab. But then we'd need revisioning for that too and it starts to be a lot of work.

Andrew Schulman’s picture

Subscribing

colan’s picture

Subscribing.

webchick’s picture

Status: Needs work » Needs review
FileSize
2.28 KB

K, I'm getting sick of looking at this and sick of duplicate reports, so here's a patch that simply removes this line from the top of the issue summary. #1230258: Make a comment for each wiki modification is probably the thing to do to fix this properly, but in the meantime it can stop looking broken.

webchick’s picture

Screenshot. You can test it on http://issue-summaries-drupal.redesign.devdrupal.org/ (user: bananas/bananas)

No more incorrect revision info

xjm’s picture

This seems like a good interim step to me.

webchick’s picture

And NOW for a patch that doesn't stick "Issue Summary" on every node. :P~

sun’s picture

Status: Needs review » Reviewed & tested by the community

Pretty much very acceptable and appreciated stop-gap fix in my opinion.

webchick’s picture

Issue tags: +needs drupal.org deployment

Great.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -needs drupal.org deployment

+1 on the screen shot in #78. Which patch are we reviewing?

webchick’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +needs drupal.org deployment

The last one (#80). I'm confused by your question. :)

Restoring status since that has the smell of a cross-post.

jhodgdon’s picture

yes, was a cross-post, sorry! I just wasn't sure if the patch in #80 is what you used to make your screen shot in #78 and the test site?

xjm’s picture

Aye, #80 is currently applied on the test site, and produces the behavior in the screenshot above. (The difference between #78 and #80 is that the issue summary header was mistakenly moved outside the project issue case and so applied to all nodes, but that is fixed now on the test site with #80).

dww’s picture

YAY!!! Thanks webchick. You rock. We should have done this months ago.

Committed and pushed.

Sadly, due to some jenkins snafus I can't actually deploy this right now, and I need to run to class. I'll deal with it later tonight to get this live.

dww’s picture

Status: Reviewed & tested by the community » Active
Issue tags: -needs drupal.org deployment

Deployed. Yay!

Back to active for actually dealing with this issue... not sure what to do with it, and my head is in many other things right now.

jhodgdon’s picture

Priority: Major » Normal

At this point then, I don't think this is such a major issue - more of a normal issue. :)

salvis’s picture

Hmm, the OP is "Posted by webchick" even though sun authored it and Jennifer edited it last. Angie never touched the OP. I wouldn't call that normal, but it looks like trashed data, not something that can/should be fixed in run-time code.

Should the authorship of issue nodes be fixed in a batch run that sets it back to the uid of the first revision?

Not displaying the name of the last commenter is certainly an improvement, but since Jennifer went through 90 comments, digested them, and edited the summary, she would deserve some recognition for that. I'm pretty sure that was the purpose of the "Revision # by ..." line, and it's still a very valid concern.

(It'd be nice to also display the number of / link to the comment which comes after the last edit of the summary, so the reader knows where to pick up the thread, if he trusts the summary to be accurate.)

xjm’s picture

#90: No, the original post was by webchick. All the authorship in the revision history is totally bogus, which is why this issue is active now. :)

jhodgdon’s picture

Well. Saying the revision history is totally bogus is not quite accurate. It just doesn't reflect who edited the node body (as you'd expect) -- instead it's a totally accurate history of who last commented on each revision (and who therefore might or might not have changed other issue properties). But of course that is not exactly useful or what we want. :)

pillarsdotnet’s picture

Yeah, the revision diffs unfairly blame other people for my typos. :-(

webchick’s picture

silverwing’s picture

re#94 - I mass deleted comment spam, then noticed the issue was vandalized. I went to revert the node and noticed the last revision (the spam) was attributed to me.

klonos’s picture

This is not addressed still and I really hate taking credit as being the revision creator in place of the person that actually went through the trouble and spent the time to do the summary simply because I went and corrected a minor typo they made :/

...hiding the wrong author info is simply "sweeping dust under the carpet" so that people don't notice/complain about it - not actually addressing the issue at hand.

dww’s picture

Status: Active » Postponed

Fixing this for real in D6 is probably going to be too much work. This will be fixed automatically in the D7 port. See #1545922: [META] Issue page redesign for more.

dww’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

...I realize that this is postponed and it is about how posting a comment changes issue properties, but I just had a WTF in an issue revision comment for #1702354: Apache Solr Multilingual 7.x Roadmap:

wrong user in issue revision info

Is this the right issue to report this problem to?

drumm’s picture

Status: Postponed » Closed (won't fix)

D7 avoids this problem by not updating the issue node on comment and we should be able to launch in a few weeks.

drumm’s picture

Issue summary: View changes

added status information

  • Commit a6fea67 on 6.x-3.x, 7.x-3.x-dev authored by webchick, committed by dww:
    [#1217286] by webchick: Removed bogus revision info from issue summaries...

  • dww committed a6fea67 on 2299191-beta_project_issue_project_searchapi authored by webchick
    [#1217286] by webchick: Removed bogus revision info from issue summaries...

  • dww committed a6fea67 on 2322267-migrate-country-field authored by webchick
    [#1217286] by webchick: Removed bogus revision info from issue summaries...

  • dww committed a6fea67 on 2322267-migrate-gender-field authored by webchick
    [#1217286] by webchick: Removed bogus revision info from issue summaries...

  • dww committed a6fea67 on 2348121-missing-bio-information authored by webchick
    [#1217286] by webchick: Removed bogus revision info from issue summaries...

  • dww committed a6fea67 on 2350591-not-spammer-role authored by webchick
    [#1217286] by webchick: Removed bogus revision info from issue summaries...

  • dww committed a6fea67 on 2322267-bakery-sync-country authored by webchick
    [#1217286] by webchick: Removed bogus revision info from issue summaries...