Problem/Motivation

Communication is at the very center of everything we do as a community, and issue queues are the primary mechanism through which we communicate. However, the Drupal contributor community collectively loses hours of time each and every day in discussions, because there is no consistent way to find out what the current state is of any given non-trivial issue.

Once an issue hits about 20+ replies or so, a number of things start happening:

  • Contributors not yet part of the discussion shy away from it because it's too intimidating. This dramatically reduces the number of people who can fix it.
  • Where new contributors do wade in and get involved, they often just skip to the bottom of the discussion and start chiming in there, often repeating information that's already been hashed through 20 replies before. This is frustrating for all involved, and makes the issue even longer when this is pointed this out.
  • Patch reviewers and committers both must read the entire issue top to bottom to understand what's happening. This dramatically increases the amount of time it takes to perform patch reviews and commits, leading to patch stagnation.
  • Even in the rare cases that someone has taken it upon themselves to make an issue summary as one of the comments in the thread, the trick then becomes finding that issue summary in #99 of 128 comments, and trying to tell if it's still up to date given the other 10-15 replies below it.

This is fundamentally the single biggest Drupal community performance issue, and bites us absolutely everywhere, regardless if the problem trying to be solved is in core, contrib, or Drupal.org itself. Fixing it will make solving every single problem we solve thereafter (including improvements to how issue summaries are done) much, much easier.

Proposed resolution

Originally, the proposed fix was to flag one or more individual comments as an "Issue summary". However, this plan was abandoned for the following reasons:

  1. In order to make a small correction/clarification to someone's issue summary, one has to re-copy someone else's comment and then paste it again. If someone else wants to make a small clarification to that, they have to re-copy and paste it again as well. This needlessly elongates the issue and makes it more sprawling and difficult to read and understand.
  2. There's also no way to "diff" separate comments to see what was added or changed between revisions.
  3. And finally, the infra team has nixed the proposal to deploy Flag module for comments due to the unacceptable performance hit on d.o (see #60).

Previous attempts have also involved a separate summary node, node-referenced to the issue, as well as a single "summary" field on an issue that's editable, but this too required putting new code on Drupal.org and thus got stalled.

Therefore, what's being proposed instead is the simplest thing that could possibly work, namely to make initial issue bodies themselves editable (essentially "wiki" pages like handbook pages), and use the initial post as the issue summary. This has the following advantages:

  1. Does not require new modules to be tested, approved, and deployed to Drupal.org, which has killed the last two attempts (at least) of fixing this issue.
  2. Easy to tell when a summary has been edited, what was changed, when, and by whom thanks to node revisions + diff module.
  3. No fanciness; works with via same standard permissions as handbooks do, which has already been tested and approved for use on d.o.

The following concerns are acknowledged, but being pushed to follow-up issues so we can gather data on actual usage sooner:

  1. Shepherding of issues edits on top of maintaining queues for contrib.
  2. No comment stream integration means dashboard widget behaviour is inconsistent.
  3. No comment stream integration means updates do not flow into discussion.
  4. Original post retention is not enforced and could be relegated to revision hunting.

Remaining tasks

This code has been deployed! YAY! :D

However, doing this uncovered a bug with Project Issue module (actually with node_save()) that needs to be fixed:

#1217286: Posting a comment changes too many issue node revision properties

Not a blocker, but unit tests are also highly desired from the Project issue module maintainer for the following issue identified during testing:

#1178288: Data loss of uploaded files when re-editing issue

User interface changes

  • Additional description added to top of issue form field to explain the mechanics.
  • Display of issue nodes has changed slightly:
    - Heading of "Issue summary" at the top
    - "Revision X by USERNAME on DATE" at the top to indicate it's been edited.
    - Link "Revision X" to the diff if there are more than one revision.
  • Inline notifications that the summary has changed

API changes

None.

Follow-up issues

  1. #1217646: Tweak UI of issue summaries

Unrelated issue commonly confused with this one

  1. The Change records for this issue sidebar is not related to this issue.
    See #648218-161: Make API changes in Drupal core be nodes
    and #1217118: [meta] Followups for API change nodes

Original report by dww:

Splitting this off from #569552-69: Provide a mechanism for issue meta discussions from rfay:

Bojhan, DamZ and I just talked a bit about this and DamZ has an alternate proposal:

He suggests a checkbox on a comment which marks it as an issue summary, and then the summary at the top of the node would have links to issue summaries which have been posted. It also does not require adding comment_driven, which he is opposed to.

I would be fine with this. It allows access to good issue summaries from the top of the issue, and allows multiple issue summaries (which might be associated with different cycles within the issue).

The older issue had been moved into a related but separate problem of being able to highlight or flag specific comments as issue summaries. The idea is that it's hard to tell which comments in a large issue are a useful place to read what's up with the issue, instead of trying to wade through dozens (sometimes hundreds) of replies. However, issue summaries could be useful for all issues, not just the problem of meta issues to coordinate a set of related tasks (which is what #569552 is really about), so I'm splitting this off into a new issue.

DamZ wrote a little module for this: https://github.com/damz/comment_highlight

jhodgdon asked if this couldn't be implemented using http://drupal.org/project/flag -- good question

donquixote pointed out "Now everyone can tag their comment a "summary", but i assume in most cases people will be honest."

Right, that's a huge concern I have about this approach. See:
#917880: Write a handbook page about how issue tags are intended to be used
#1023108: Make issue tags less visible by default and link to the documentation on how to use them
for example. Ever since the "Issue tags" field is quite visible on issue comments (I'd really love to know when that changed), people add worthless tags all the time because they can (and think they should). While I'm skeptical that "in most cases people will be honest", I have absolutely no faith that most people will understand whatever this new checkbox is -- I predict tons of misuse (even if not abuse as such).

So, if we go down this road, we need a way to uncheck the box or unflag. And, I think this should be hidden in the default comment UI (at least hidden behind a collapsed fieldset or buried under a vertical tab, etc). Yes, I think you should have to click (at least) twice to use this feature.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

I agree with Bojhan, (#569552-75: Provide a mechanism for issue meta discussions), there does need to be a textual representation of whatever style is assigned to a summary comment. This could go after the comment
number, or after the "New" text for new comments. The first is preferred as it will be more easily noticeable by screen-reader users who otherwise need
to listen to the entire line of text to find the "Summary" identifier.

Everett Zufelt’s picture

Issue tags: +Accessibility

Tagging

jhodgdon’s picture

I don't understand why this was split off, but whatever -- I think that other issue had evolved to the point that it was about any issue and not just "meta" issues?

Anyway, I definitely think issue summaries would be useful for all issues. One of the reasons is that to announce/collect API changes, we need to have a summary of the issue. Another is that if you do a search for some problem you are experiencing and find an issue that might or might not be relevant, the ability to jump to an issue summary and figure out what the issue is about would save a lot of time. We would probably need to have some guidelines or a template for an issue summary, like saying it should (eventually, and/or where appropriate) include a summary of the observed bug or requested feature, the root cause of the bug if it's a bug, what was done to resolve it, and if it is an API change a summary of the change.

jhodgdon’s picture

Regarding how to do this:

One advantage of using the Flag module to do this vs. a checkbox at submit-comment-time would be that a comment that had been marked as a summary could be un-flagged if it wasn't any longer relevant as an issue summary or had been marked in error, without having to edit the comment. As a doc admin, I have permission to edit comments, but I don't think regular users do?

One disadvantage could be that random people could flag/unflag comments irresponsibly or maliciously, at least it seems like they could?

jhodgdon’s picture

If this is added as Flag or a checkbox, we should provide a link to a page describing how to write a summary. We have links like this now in the issue settings area:
Descriptions of the Priority and Status values can be found in the Issue queue handbook.

rfay’s picture

@jhodgdon, or anybody else, do you have a sandbox where @DamZ's module could be displayed? If we're going to pursue that angle it would be nice to have a public place to display it.

One problem with using flag is that I don't think it's yet deployed on d.o, so there is hoop-jumping to be done.

dww’s picture

There's just as much hoop-jumping to be done to deploy DamZ's module as flag (if not more, since he'd have to move it into d.o CVS). Flag's already been approved (for use in #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment), that's just not ready yet (*sigh*) so flag's not deployed.

rfay’s picture

Well Yay for flag! That will be great to have.

klonos’s picture

jhodgdon’s picture

rfay (#6): I have a development d.o server that we could try out a Flag solution on. Ping me in IRC in a few hours and maybe we can get something going. It sounds like if we can demonstrate a need/use for Flag, we can turn it on for d.o.

jhodgdon’s picture

Status: Active » Needs review
FileSize
32.87 KB
1.08 KB
742 bytes

Should we be moving this to the d.o module queue, since that is what we're talking about as a solution now, rather than being on project issue? Although this would also need a bluecheese patch. See below...

So... I have the Flag idea deployed on my dev d.o site. It took about 30 minutes to build, including theming (and I'm not a major themer). Open to suggestions on how to make it better. For now, you can go to
http://jhodgdon.redesign.devdrupal.org/node/942782
and scroll down to see it in action.

What I did:
- Turned on the Flag module.
- In the Flag UI, added a comment flag called "issue_summary" that applies to Issue node comments. Made it flaggable/unflaggable by any logged-in user.
- Modified the preprocess comment function in the template.php file so it checks to see if a comment has been flagged as an issue summary. If so, adds the text "Issue Summary" as a variable, and adds a CSS class to the comment as a whole.
- Added some CSS to turn the comment background a light red color (one of the colors listed in styles.css), and to make the Issue Summary text bold.

Note:
- The flag module apparently gives you a default "bookmarks" flag. If we deploy the Flag module on d.o, we would probably want to remove that flag or set its permissions so no one can use it, until we decide what to do with it. It also sets up some default views, and these would need to be disabled.

Attachments:
- Screen shot, so you can see what it looks like when you are logged in and therefore have permission to flag comments
- Patch for bluecheese... I just did a bzr diff -- not sure if the patch will apply, so if anyone can clue me in as to the bzr commands I should use to make a patch, that might help.
- Export of the flag I created in the UI. The flag module says you can either import this in the Flag UI or "Exports can be included in modules using hook_flag_default_flags() or using the Features module.". I haven't tried either.

Marking this as Needs Review even though it's not technically a patch yet...

jhodgdon’s picture

Oh. I just realized that I ignored Everett's accessibility suggestion in comment #1 above, where he said it would be preferable to put the "Issue Summary" text on its own line so a screen reader wouldn't have to listen to the whole line to figure out it's an issue summary. That is easy to change in the comment.tpl.php anyway.

There is also currently not a way to jump to the first issue summary, or to make a list of issue summaries, besides doing control-F in the browser and searching for "Issue Summary". It would be easy to make a list with a Views block, but I'm uncertain about where such a block would be displayed. Any ideas?

jhodgdon’s picture

If anyone would like to be able to log into my dev box to try this out, you can ping me in IRC or use my d.o contact form to email me, and I can reset your password to something usable.

One other note: At the moment when you flag/unflag an issue as a summary, the comment rendering and styles do not change (as things are now). You only see the light red background and the text that says "Issue Summary" if you reload the issue. I think that is OK, because if you just flagged it, you are aware that it's an issue summary -- the text/styles are so that others coming to the issue can find the issue summary.

jhodgdon’s picture

dww: are the "Jump to most recent comment" and other links at the top of issues generated by Project Issue * or somewhere else? Can we add to them?

dww’s picture

@jhodgdon: re #14: yes, they're from project_issue. See project_issue_internal_links() in includes/issue_node_view.inc. We should probably just add an alter hook to that. That should be pretty trivial, and then makes it easy and clean to add to these via a feature or other code in drupalorg.module.

No time to review all the other work yet.

We should either click this whole thing together as a feature, including the exported flag, any related views, etc. Or, we should put it all in drupalorg (e.g. in drupalorg_project).

Thanks,
-Derek

jhodgdon’s picture

FileSize
11.64 KB

So far the only thing to package is the feature, as the other patch is for Bluecheese. For me, it will be easier to do as a patch for drupalorg.

But speaking of Views... Here's a view to replace the dreaded module update 6/7 page:

link to this view

I'm not sure why the Component filter is not working. I have a project argument, and it's making the Version filter work, but not the Component filter. Any ideas?

Anyway, the thought was that going forward, instead of trying to maintain the huge and unmanageable Module Update 6/7 page, we would instead provide a view that lists the issue summaries, and you could filter by the "API Change" tag to find all the summaries of issues involving API changes.

Thoughts? Any idea how to fix it? I'm sure it could be made better...

(Attached is an export of the view.)

rfay’s picture

Nice thinking and nice work!

I'm not sure at all that the issue summary can be shoehorned into the module update page, as it's probably too granular. Oftentimes many issues should be combined into one update note. And one issue might have several summaries (and we'd theoretically want the last one, although I've seen exceptions).

Great stuff. BTW, the theming doesn't seem to be working on http://jhodgdon.redesign.devdrupal.org/node/942782. All I see is "unmark this comment", no theming to mark off the comment. And no list of summaries at the top, if you did put that in.

jhodgdon’s picture

When I go to that page, I see the theming.... hmmm... (logging out)... Yes, I'm still seeing the theming if I'm logged out. You might try reloading the page and/or your browser cache. Note that the issue summary doesn't show up themed when you first flag the issue, only on page reload. See #13 above.

The "jump to" links at the top of issues haven't been done yet, which is why you aren't seeing them. See #12/#14/#15

Regarding the module update page replacement view... The module update page is almost completely unusable as it is (in my opinion), and even now we have about 20 issues that have been tagged for update doc that haven't been written up for D7. So is it possible that we could do something with comment flags to replace it, even if this exact solution isn't it? Maybe we just need a second flag for "API change", and people could write up the API changes as issue comments?

jhodgdon’s picture

Actually, I like that last suggestion. The idea would be:
- Second comment flag for "API Change Writeup".
- A different color background for these comments, like maybe green or blue, and text saying "API Change Writeup" as well.
- A view that collects these API change writeups by version to replace the Modules 6/7 type of pages.
- A "Jump To" link at the top of the issue for these as well as summaries

This way:
(a) The API change information would be right there on the issue (or at least one of the issues, if several issues' changes were grouped together). This means people reading the issue could find out what API changed, and people coming from the API change view could read more about the issue, find the issue summary, etc. Both good. :)
(b) People familiar with the issue could write up the change notice without too much difficulty and it would be collected automatically.

rfay’s picture

Clearing the browser cache got me the theming, thanks.

Still no list of summaries at the top, right?

Love your idea of doing the API change separately. Excellent. You're killing all kinds of birds with this simple solution.

jhodgdon++

Everett Zufelt’s picture

I think I would prefer to see a separate node for API changes, which could reference all issues that are related to the change, and where someone familiar with the change can provide a brief summary, with links to the relevant issue summaries. This is a bit more work for people documenting API changes, but fulfills the need for multiple issues to constitute a single change. It also makes it a little easier for people to know what a change is. Do I activate the flag if I am suggesting a change, agreeing with / disagree with, after it has been submitted? Having this in it's own node makes it a bit more clear.

jhodgdon’s picture

I see what you are saying in #21, but my thought would be to add the API change-tagged comment on only one of the relevant issues, not all of the relevant issues. And:
- The API change comment can and should include links to the other relevant issues and/or their summary comments.
- The API change comment should provide a summary of what the API change is.
- Using a comment is handy -- it is already in the issue, where people familiar with the issue can work on it. Making it be a separate node adds a burden to contributors. They are not currently writing up API change documentation much if at all, and we need it to be as small of a burden as possible so that they will actually do it.
- I think I would make the flag link say "Mark this comment as an API change notification", to make it as clear as possible that this would be going into the API change page.
- We should probably make some templates for issue summaries and API change notifications (and have links below the comment form) so that people have guidelines as to how to write them up.

Everett Zufelt’s picture

I see what you are saying in #21, but my thought would be to add the API change-tagged comment on only one of the relevant issues, not all of the relevant issues. And:

* How will we ensure that this is done in practice?

- The API change comment can and should include links to the other relevant issues and/or their summary comments.
- The API change comment should provide a summary of what the API change is.
- Using a comment is handy -- it is already in the issue, where people familiar with the issue can work on it. Making it be a separate node adds a burden

* AFAIK, only the user who posts a comment can work on it, this means it cannot be edited / corrected / improved by others.

klonos’s picture

@rfay, #17:

...one issue might have several summaries (and we'd theoretically want the last one, although I've seen exceptions)

This might not prove to be an issue at all. Multiple summaries might be a good practice as an issue progresses. Think of them as a here's-what's-been-done-so-far or this-is-what-lazy-people-read-if-they-don't-want-to-go-through-the-whole-issue-history.

OTOH, since (or when) comments can be flagged and then unflagged from being a summary, when a new one is written we can somehow mark previous ones as obsolete, but I cannot think of how we could do this. Now that I think of it what we actually need here is some sort of taxonomy mechanism for comments plus a way to filter them. This way we could narrow down the list of comments based on various criteria (like 'contains patch', 'is summary', 'API change' etc) - a.k.a. provide faceted/solr search on comments.

As for linking to summaries, comments that are flagged as summaries can somehow be 'promoted' at the very top of the page, right after the initial issue report (comment #0). This way there will be no need to link to them.

klonos’s picture

...comments that are flagged as summaries can somehow be 'promoted' at the very top of the page...

Of course these 'special' summary comments must be somehow excluded from receiving a comment number or else it would be weird to have say comment #13 placed before comment #1.

jhodgdon’s picture

#24/25 - Let's keep this simple. I don't think we want to get into tagging comments with a complex taxonomy. I think we just want to flag some comment(s) on an issue as "This is an issue summary", and make them visually jump out at you, with a link to jump to the latest one. Anyone can flag/unflag them, as I have it set up now.

Regarding #23 - I see what you are saying about separate API change nodes. Being able to edit them and have revisions etc. is a good idea. Presumably the process if we used issue comments would be that if you wanted to revise the API change notification comment, you would just copy it and submit it as a new comment, then flag the new comment as being an API change notification, and unflag the previous one.

If the change notification is a node, then we'd definitely have to set up some links so that you could easily jump to any API change notification nodes that referenced the issue you're looking at, and vice versa. That would involve making a node reference field on the API Change node that would point to one or more issues, and then making a view to display on the issue node that would list all API change nodes that reference this issue. It's quite a bit more complex than the "do it as a comment" solution. I'm not saying it can't be done, but it's not as easy to get a patch created and deployed.

dww’s picture

API change notification nodes were discussed at length at #648218-3: Make API changes in Drupal core be nodes. I'd rather not complicate this issue with that problem. Let's leave this one for issue summary comments, and either use a separate issue for API change comments or just use #648218 for doing it via a separate node type. That way, whatever patch we're dealing with in here will be easier to review/understand/test/deploy.

Thanks,
-Derek

jhodgdon’s picture

Good idea dww. I'll table the discussion of API notifications here.

So what needs to be done here for issue summaries is:
a) We don't need the view discussed in #16.
b) We do need the bluecheese patch and the flag export from #11. It needs to be modified so that the Issue Summary text is more accessible to screen readers. I think I'll try it at the beginning of the comment submitted by line instead of the end?
c) We need the "jump to" link at the top, which should jump to the most recent issue summary. To do that, I need to make a project issue patch to add an alter hook to the area at the top, and a drupalorg patch to do create the link. I'll put the flag export in the same area of the drupalorg module at that time.

That shouldn't be too much work, so I'll try to work that up later today.

Everett Zufelt’s picture

@jhodgdon

Yes, beginning of submitted by line would work well.

jhodgdon’s picture

Everett: Will do, thanks for the feedback. This way the line would read "Issue summary - Posted by ...", which I think would be fine.

jhodgdon’s picture

OK, here is pass 2 at this functionality. Check out
http://jhodgdon.redesign.devdrupal.org/node/942782
again to see it in action.

What it does:
- Gives you a link to flag/unflag each comment on a project issue as being an issue summary.
- Upon page reload (this is not dynamic after flagging/unflaggin), comments flagged as summaries are rendered with a light red background, and have the text "Issue Summary" as a prefix to their "submitted" line.
- In the Jump To links at the top of the issue page, there is a new link that takes you to the most recent issue summary. (For testing purposes, I marked a few comments on this issue as summaries, so the link should be to the most recent one.)

Here are the patches needed to make this work:
a) bluecheese - I patched this via bzr on my dev site because I can't seem to cvs checkout the bluecheese theme project? Hope that's OK. The patch affects the preprocess_comment function and the comment.tpl.php template.
b) project issue - patch adds a drupal_alter as suggested by dww in #15 above.
c) drupalorg - patch implements the drupal_alter to make the jump to link, and also implements a Flag hook to define the flag.

If you install these 3 patches on d.o or a dev site, you will additionally need to enable the Flag module, and then go to admin/build/flag and disable the default "bookmarks" flag, and enable this new "issue_summary" flag (if it's not already enabled -- I'm not sure since I built it from the UI and then added it as a hook).

Comments welcome...

rfay’s picture

I would really really like to see a list of summaries or a pointer to the last summary in the issue summary detail at the beginning, with the assigned, status, etc. The whole idea is to make it quick to grok an issue, and I think this would help.

I apologize for not saying this earlier. Somehow I got the impression it was in the plan.

jhodgdon’s picture

RE #32 - There *is* a link that goes to the latest issue summary in the "Jump To" links at the top of the page, along with the "most recent attachment", "most recent comment", etc.

There is not currently a list of all the issue summaries. I don't think it would be all that useful, since all it could be is something looking like this:
Issue summaries: #5, #7

So you would need to jump down/up/down/up to see all of the summaries. To me, scrolling to find the red areas or using control-f in your browser and searching for the text "Issue Summary Posted by" is at least as easy as "click link, use browser back button, click next link, etc." if not easier.

rfay’s picture

@jhodgdon, I hadn't seen the link to most recent summary. That works for me. Thanks! It's not very prominent, but not sure it needs to be.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

IMO this one is good to go. We might need to adapt it in the future, as always, but it's a great start.

Damien Tournoud’s picture

We are going to have to verify the performance impact on using flag on this. Last time I checked, flag does at the minimum one query per flaggable comment displayed on a page (it used to be 2... I shot one).

rfay’s picture

@DamZ, The issue about approving flag module on d.o is #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment. I haven't taken the time to grok the whole thing, but it's my understanding it's "approved", but you should chime in there with your concerns.

dww’s picture

@rfay: No, that's not an infra issue about deploying flag. That's another specific use-case. DamZ is chiming in here with his concerns about benchmarking this particular use-case, which is valid. Or we open a separate infra issue specifically about deploying flag like we often do. But no one should reply at #34496 about flag's performance for #1036132. ;) Thanks.

rfay’s picture

OK... thanks.. I was responding to your statement in #7 which I thought clearly stated that this would be a pre-approved pathway:

There's just as much hoop-jumping to be done to deploy DamZ's module as flag (if not more, since he'd have to move it into d.o CVS). Flag's already been approved (for use...

So let's get together a clear plan for how we'll get this done. What do we have to do?

dww’s picture

Status: Reviewed & tested by the community » Needs review

flag was approved in principle. But if there are specific performance intensive use-cases that are likely to cause infra problems, those need to be addressed. In the case of the other issue (subscribing to issues) the need is so great that killes basically agreed we have to do it and solve whatever performance problems arise. I don't know that this issue qualifies with the same level of "doesn't matter how bad it gets, we'll just have to fix it" approach.

So, I believe what needs to happen here is some benchmarking on the dev site. For example, disable flag.module, enable devel, and load a big issue page with a lot of comments, and see the total # of queries and time spent querying. Then, turn all this stuff back on, reload, and see the new values for the same metrics. Post those here. That'd be a start.

Thanks,
-Derek

dww’s picture

p.s. To clarify, after turning on all this functionality, also be sure to flag some comments as summaries and post the query count. In fact, it'd be nice to see values for pre-patch, patch and nothing flagged, patch and N summaries flagged. Thanks.

jhodgdon’s picture

OK, I'll see what I can do about benchmarking, since as far as I know my dev site is the only one with the patches in #31 applied.

I wish I had known this was a possible issue before I spent the time to make those patches. Sigh.

jhodgdon’s picture

Here are some benchmarks.

I tested with two fairly typical issues: 947872 (33 comments, with 2 flagged as summaries) and 96887 (18 comments, with zero flagged as summaries).

I did 3 page loads, after discarding the first one (query counts were higher on the first load, presumably due to some menu or other caching?). Here are the query count (QC), query time (QT), and page load time (PT) results, with the flag module turned on or turned off (I didn't revert out the rest of the patch, just turned the Flag module on and off). Note that QT and PT were *highly* variable!

Issue  Flag QC  QT   PT
942872 yes  556 1671 2626
       no   421 1532 2523
 96887 yes  414 1491 2330
       no   339 1398 2199

It looks like it adds about 75 queries and 100 ms to the query load for the smaller issue, and 130 queries and 140 ms to the query load for the larger issue.

Bojhan’s picture

Is there a image to review?

jhodgdon’s picture

Bojhan: see #31 above

Bojhan’s picture

Background
Instead of using red for the background, I would suggest using a light blue #d1e3ef or light yellow #e6fcb6. Because using red here, kind of conflicts with the calm tone that we want to portrait in our color scheme.

Label
Try using a red as used in "updated" and "new" which is #fd0036 I think, it makes it stand out more and is consistent with other kind of messages. Additionally we can probably shorten this to only say "Summary" the issue part seems fairly implied.

jhodgdon’s picture

RE - colors and text from comment #46 - OK.

On my machine, "New" for new comments when viewing an issue shows up as black text on a green background, and when viewing a list of issues (such as the Your Issues page) it shows up in red, but point taken.

Gerhard Killesreiter’s picture

I am not happy wrt seeing the query count going up all that much. The queries might be rather simple, though, can I get samples?

jhodgdon’s picture

Status: Needs review » Needs work

Discussed this further with GK (killes) on IRC. The way this is working now, it is asking the flag module "is this comment flagged as a summary" for each comment. He thinks it would be better to do it once at the issue level and cache statically. I agree. Back to the drawing board for the patch...

I also need to get the test site running again. I think I disabled Flag when I did the performance checks and never turned it back on.

jhodgdon’s picture

FileSize
818.41 KB
4.55 KB
52.47 KB

I have the test site up again (thanks killes and basic`)... Here are screen shots of the Jump To section and what an issue summary looks like currently. I'll adjust the programming and the colors as per the last few comments and see if I can get it working. Anyway, here's the "before" shot.

Also, here's a dump of the queries from
http://jhodgdon.redesign.devdrupal.org/node/942782
as requested by killes.

jhodgdon’s picture

FileSize
412.05 KB

Here's an HTML copy of the page with the queries shown.

jhodgdon’s picture

Queries:
a) The Jump To section at the top of the page has a query where it finds the latest comment marked as a Summary.
b) For each comment, Flag module makes a link to either flag or unflag as a summary.
c) For each comment, bluecheese (with my previous patch) is checking whether it's flagged and if so, adding a CSS class and some text to the comment. Via a call to $flags->is_flagged($vars['comment']->cid with $flags = flag_get_flag('issue_summary')

I can probably make (c) more efficient by doing some caching as suggested by Killes, but not (b) since that is in the guts of the Flag module. The queries are the same for (b) and (c) incidentally, but I don't know that Flag is being intelligent about caching them.

jhodgdon’s picture

RE #52 - And by the way (b) is being done on EVERY node page for every comment, not just issues.

jhodgdon’s picture

More IRC discussion...

So there was an alternate proposal by DamZ in https://github.com/damz/comment_highlight
The problem is that it only allows you to "highlight" a comment when you are editing the comment. I think we really want the functionality of Flag, so that any user can flag any comment as "this is a summary", or unflag it if a better summary comes along.

Back to Flag, we might be able to make it more efficient. Currently it's doing a query per comment displayed on every node, to see if the user has flagged it [calling flag_get_user_flags()]. The thought would be to add a function like flag_preload_user_flags($nid) that would do one query to load the flag status of all the comments in one fell swoop. Then flag_get_user_flags() would look at this cache to see if it already has information rather than doing a separate query on all nodes.

Obviously, details would need to be worked out, but it might be workable.

Bojhan’s picture

Status: Needs work » Needs review

It seems this issue is stalled for a bit, jhodgdon provided data - can we get feedback whether this performance hit is acceptable or if we need to adjust?

drumm’s picture

projectissue-alterlinks-31.patch from #31 looks like a straightforward & generally useful patch for project_issue. dww - can you commit that, and we can move this issue to the Drupalorg module?

dww’s picture

Sorry for the delay. Yes, the project_issue patch in #31 was indeed a simple win. The only problem it had was it was missing a hunk for project_issue.api.php. However, that's understandable, since that file didn't exist. ;) I fixed that, committed both, and pushed upstream:

http://drupal.org/commitlog/commit/1894/b3165b61e58c6d64ca66e878ddf5b2b0...

No time now to review the rest of this, but at least that should no longer be blocking anyone...

Cheers,
-Derek

jhodgdon’s picture

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

Thanks dww! Moving to drupalorg project (although one of the patches in #31 is for bluecheese too).

The next question:

We have a working way to flag issue comments as summaries. It involves the flag module and an increase in the number of queries, as detailed in #43, #50, and #52 above. We need an answer from the infra team about whether this is an acceptable load.

- If the load is acceptable, as per Bojhan in #46, the patches need a little tweaking for style/color/wording.

- If the load is not acceptable, then either Flag needs some updates to make the load more acceptable (see #54), or the equivalent functionality needs to be built without Flag.

dww’s picture

I'll have to defer to nnewton + killes on if the query hit from flag is acceptable. It's pretty sucky, and it sure seems "too much" to me, but maybe they think our DB server can handle it and they're not worried. Lots of very cheap queries isn't as big a deal as even one terrible one, so the specific queries added by flag probably need to be EXPLAIN'ed and understood. If it's easy (and a general, obvious win) to optimize flag itself, I'm sure no one would object... ;)

That said, I'm getting a bit of cold feet about plowing ahead with this specific implementation plan given all the ideas and mockups being generated over at the Prairie Initiative. I'm worried that if we end up rolling this out and try to get people to use it, it's going to be harder to do something even more far-reaching and better. Maybe that's a bogus fear, but I wanted to at least mention it. I want to be aware of churning our collaboration tools too much, pissing off users, etc. I certainly am not going to veto this, I just wanted to express my uneasiness as it currently stands. It's probably colored by staying up until 4am last night reading and contributing to Prairie g.d.o posts and getting excited... ;)

killes@www.drop.org’s picture

Flag is indeed very sucky and I don't want to deploy it in the current state, even if the indiv. queries are indeed pretty harmless. There are simply too many and it seems as if the internal cachin mechanism would have be broken.

jhodgdon’s picture

Status: Needs review » Postponed

Thanks for a definitive answer on the query load.

Regarding Prairie, it does look like they're discussing what the issue page should look like at
http://groups.drupal.org/node/136174

So I guess we should maybe postpone this until consensus is reached?

rfay’s picture

I dearly hate to see this good work get stymied.

I felt like @killes certainly left the door open for some flag performance improvement. Oh well.

jhodgdon’s picture

rfay: Well yes. We could improve Flag... What's the time frame on the Prairie Initiative figuring out what the issue page should look like? It seems silly to put more effort into getting this deployed (and more importantly, adopted by the dev community) if a different solution might be chosen and deployed pretty soon.

rfay’s picture

You're right of course. I'm just a fan of doing what can be done when it can, rather than waiting for a silver bullet. And the work you've already done was fantastic. And of course I have no idea how much would have to be done to flag to make this better, but thought you'd mentioned that you had some idea.

dww’s picture

I also want to add my disappointment that all this great initiative and effort from jhodgdon isn't "getting anywhere". I'm keenly aware how rare and precious it is to have people not just inspired to help but actually doing it. So, I'm definitely not thrilled that this isn't moving forward. But, overall, I think it's best to give the Prairie a little while to figure out what it's doing and see if we can make more far-reaching and fundamental fixes to the underlying problem this issue is trying to solve. Therefore, yeah, I grudgingly agree that "postponed" is probably the right status for this at this time.

Thanks and sorry!
-Derek

jhodgdon’s picture

No problem - I didn't put all that much time into this "great initiative". :) I have limited time though (of course) amongst my other responsibilities in Drupal-land, so I am unlikely to jump into Prairie much.

What I'd like to make sure of is that we don't wait too long. We could really use a solution sooner rather than later. If Prairie doesn't come to some conclusions soon, then I'd be willing to put a little time into Flag and see if we can get the query count down.

I also just filed an issue on Flag to track this feature request of making the queries more efficient, which seems like it would be a good idea for Flag whether or not this issue comes back to life.
#1133956: Improve efficiency when displaying lists of flaggable comments

jhodgdon’s picture

I am coming to the conclusion that issue summaries should *not* be comments.

I wrote up a trial balloon page on what an issue summary should consist of:
http://drupal.org/node/1155816

It seems to me that the better way to go about it would be to make the Issues themselves editable by any authenticated user (as most book pages are), and have that be the basic problem/motivation field on my proposed summary template. Then we would have additional CCK fields for the other items in the summary. If we enabled node revisions for the Issue, we could easily see how the issue has progressed. And it seems to me that moving the issue title, project, and other Issue fields so they are edited when you mean to edit the issue (not every time you file a comment) would also be useful... but that may be another discussion.

jhodgdon’s picture

Also note: http://groups.drupal.org/node/136174 has a discussion about issue summaries on it

It includes two different kinds of issue summaries, as I see it: in-progress (what's going on on the issue and what's its current status) and ready-to-commit (the kind I am talking about on http://drupal.org/node/1155816)

eliza411 has been discussing UI with me on IRC just now... a proposal:

The "what is the problem here" part becomes the main issue node body and the other stuff is fields that probably can be in collapsed fieldsets (for both editing and viewing) until someone wants to see them.
Maybe one fieldset for "in-progress summary stuff" and another for "ready to commit summary stuff"

jhodgdon’s picture

My latest ideas on this are at:
http://drupal.org/node/648218#comment-4465216

yoroy’s picture

Issue tags: +prairie

Awww, belated bummer. still have to tag it, sry.

geerlingguy’s picture

Subscribe.

nnewton’s picture

subscribe

webchick’s picture

Subscribe.

I still think we should just fix this with one stinking checkbox: grant "edit any issue" to all auth users. Use the issue node as the summary. Done. Finished. Kaput. LIterally BILLIONS of lost contributor hours saved with one checkbox.

webchick’s picture

And yes, I know this would annoy people who subscribe via e-mail or see things bump in "My issues" but I think that those concerns pale in comparison to the sheer enormity of what this would do for our community's ability to work through really complicated problems and bring new contributors onboard.

(Technically, jhodgdon points out, this is two checkboxes: one for the permission, one to turn on revisions for issues. Also, we need a small migration path to toggle that flag for all existing issues. I volunteer to drop EVERYTHING else I'm working on and write that. I really, really think this is important.)

cweagans’s picture

I strongly agree with this approach. It's a relatively large change, compared to the process as it currently stands. But I think if there's one authoritative place to describe an issue and keep notes about progress and whatnot, it should be the issue node body.

tl;dr: +1.

yoroy’s picture

Priority: Normal » Critical

Right. I've been wanting to come back to this issue and stress that this a (and currently the most) critical *community performance* feature we can add to our collaboration process. In the case of flag, weigh community performance vs server performance.

I like the brute force checkbox approach even better.

rfay’s picture

All I really want is some kind of issue summary on every nontrivial issue. This is most mostly a strategic and cultural choice and the technology matters little by comparison. So I'm fine with the brute force approach.

jhodgdon’s picture

Title: Provide a mechanism to flag individual comments as issue summaries » Provide a mechanism for issue summaries

OK, here's a summary of where we stand on this:

a) We can turn on revisions easily (go to the Issue content type, and under workflow, check the "create new revision by default" box and save). Existing issues, when edited, will create new revisions, for everyone except people with elevated node perms, who can uncheck the "create new revision" box when editing if they want to, but they shouldn't.

b) We can't currently turn on "edit all issues" permission, because Project Issue has its own perms for issue nodes. That would require a patch in Project Issue to add that permission (it has "edit own issues" but not "edit all issues").

c) When I turned on revisions on my test site and then went and edited an issue, there were some problems:
- The order of the editing screen was strange (revision log box at the top)
- After saving, tags were lost
- After saving, files attached to the main node were lost
davereid has been working on these and has filed some issues:
#1178296: All file attachments regardless of revision displayed on node view
#1178288: Data loss of uploaded files when re-editing issue
#1178250: Editing a node with existing alterable tags results in data loss - USE #ACCESS!
They all have patches that need to be tested.

Anyway... other than those things, which we think can all be solved, this seems viable?

webchick’s picture

jhodgdon’s picture

Note on (c) - part about the order of the form - you have to drag the revision info down in the Manage Fields screen for the Project Issue content type to fix that.

dww’s picture

Status: Postponed » Active

With fresh eyes, yes, this is probably the best solution. I just reviewed, tested (and in one case wrote simpletests for) all the patches to projects I'm involved in maintaining, and committed/pushed all that were ready. Thanks to everyone who threw code at these problems!

A few requests/proposals for final implementation and deployment of this change:

re a) Maybe people will hate this, but I propose that if you're editing an issue, the 'Log message' field should be required. Or, at the very least, the 'Revision information' fieldset should be expanded by default to help encourage log messages. That could be a pretty trivial change via project_issue_form_alter().

d) Part of why I think 'Log message' should be required is that I think it'd be nice when viewing an issue to see markers that user foo edited the summary interspersed with the comments at the times that those edits took place. the markers could have a link to the diff, etc. That definitely shouldn't delay initial deployment here, but I think it'd be great to spin that off into another follow-up/related issue here. Thoughts?

e) drupalorg_project should also alter this form to add a form description to the "description" (body) field to make it clear that this part can be edited by anyone and that it should only be used to describe the problem. perhaps it should also say that proposed solutions are better posted as follow-up comments -- not sure if we care about the 2nd part, but the 1st seems pretty important to me.

f) Will everyone have access to the revision tab and diff? That seems really important as we go down this route. Has anyone verified that diff handles issue revisions well?

g) Given that we're hiding all issue meta data, tags, file attachments, etc, when editing an issue, do we want to add some help text explaining that to change any of that stuff you have to add a new comment? Should we want to revisit if we want to hide all that stuff when editing an issue? I'm in favor of continuing to do so, to ensure a more explicit history of the change than the revisions tab can provide, but maybe I'm being a wanna-be-librarian about it. ;)

Although things will get really weird with the test bot if people add new patches to the original post, adding more screenshots to the original post makes a boatload of sense. Maybe we should have a separate "Images" file attachment field on issue nodes that's always visible, even when editing. That could even have extra validation and magic behavior to give non-priv'ed users auto-generated img tags. That'd basically be #528682: Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions, which would be a huge win for all sorts of other reasons.

Hurray for progress! Thanks, everyone...

webchick’s picture

Assigned: Unassigned » webchick
Status: Active » Needs work

On it. Thanks so much for reconsidering this, dww! (Oh, and for the tests in the permission issue!!)

webchick’s picture

Well. Most of today was spent yak-shaving, trying to get an IDE pointed to my staging site over SSHFS and, when that failed, trying to get drupalorg testing profile running again (some success there... thanks cweagans!). So I'll have to try another stab at this again tomorrow/Tuesday. If someone gets around to debugging those issues before me, feel free!

The suggestions at #81 are all good, but IMO we need to go here for the absolute minimum necessary to get this feature deployed. Why? Because once this gets deployed, it makes it dramatically easier to implement everything else, because future people will have one quick place to go to get caught up on everything that's come before.

So to me, that means basically only e). Possibly a), too. We have some code somewhere in drupalorg module doing that for book.module already (though it drives me up the O*&@#ing wall because it flags required on preview, too. I digress. ;))

http://metrics-drupal.redesign.devdrupal.org/ has all of Dave/my patches, plus the various permissions/options set (in response to g), yes all auth users have "view revisions" permissions, even on prod). You can log in as user: jhodgdon /pass: awesome if you want to test (drupal/drupal to get past the apache prompt).

jhodgdon’s picture

RE #81 -

a/d) Log message required is fine with me, but I don't see how that is going to automatically make a comment that shows when the description was edited?

e) Description field help (on issue node body) - yes. But. I think we want more than just a description of the problem in the issue summary. Current **proposal** (open to negotiation for sure!) for what constitutes a good issue summary is here:
http://drupal.org/node/1155816

f) I believe even anonymous users have permission to view revisions on d.o... hmmm... maybe it's just logged-in users? webchick in #82 says auth users, so that's probably it.

g) Yes to help text explaining that to change other stuff you need to use a comment.

dww’s picture

Re: #83: Right, I'm in favor of getting this done ASAP and extending it, as I thought I made clear in my comment (e.g. proposing d but as a follow-up issue). However, I think (g) should also make the cut for the initial launch, since it's going to be a pretty major WTF that everyone sees an Edit tab, but most of the fields can't be edited.

Re: #84: a/d: Requiring a log message (a) just means we collect data we can usefully display later (d). That's the only connection. (d) still needs code (and a separate issue so we don't distract ourselves too much more in here). ;)

Re: #84: e: Cool. Yeah, on 2nd thought, one of the main points of this whole mess is that we want to collect the best proposed solution(s), too. So, it definitely shouldn't just be "description of the problem" or anything like that. Hurray that there's already a handbook page started on this, it'll be nice to be able to link to that in the help text (like we do for Priority, Status, etc).

yoroy’s picture

Anxiously awaiting the first screenshots! :-)

jhodgdon’s picture

yoroy: we have better than screenshots - we have a whole demo site. Not with the latest proposed updates, but your input would be welcome. Scroll up for directions...

Bojhan’s picture

Err, looking at it - I have no idea where to look. You can edit the original issue? Is that the whole idea?

geerlingguy’s picture

@Bojhan - yes, basically. The original issue will become, basically, the 'current summary' of the issue. Anyone who's an authenticated user can edit the summary and add to it, correct it, change it, etc.

Originally, the idea was to have the ability to tag individual comments as the 'current summary', but the approach webchick outlines is a bit nicer. This way the summary is always at the top. This won't be used for every issue, of course, but for 300+ comment issues, this will be a godsend in getting people up to speed on what's going on, where help is needed, etc.

Also, every edit requires a revision log message (I think), but at least creates a new revision, so past summaries can be viewed/diff'ed.

Bojhan’s picture

So how do we visually make it clear that the issue summary is up to date? Do we print a log message in the line of comments, the summary is updated? Additionally how do we make clear "this" thing on top is the summary ( a label won't do )

dww’s picture

@Bojhan re: #90: See #81 (a) and (d). I also think it'd be worth printing a "Last updated" on the issue summary, like on documentation pages.

In terms of the wider questions of letting people know the thing at the top is intended to be an updated summary at all times, that definitely needs more thought and work. Especially with 10 years of the current behavior engrained in most of our community. So, yeah, that definitely needs some work. But, we don't *have* to solve everything in the first pass. We can get it 80% right and then iteratively improve the final 20% over the next weeks/months.

webchick’s picture

I'm afk for most of the afternoon today, but should be able to do some hackery this evening (4-5 hours from now).

If people want to iterate on some possible mockups between now and then that address a few of these concerns with simple text changes/reprinting of variables/etc. then I'd be happy to give a crack at implementation.

dww’s picture

p.s. Oh yeah, another WTF that I guarantee will cause problems with this approach: as soon as someone wants to embed images in the summary, they're going to set the input format to "Documentation" which is going to hide the edit tab for 98% of users. Other than #528682: Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions (so that you don't need elevated perms to display images) I'm not sure there's much we can do about this, but I wanted to at least raise it in case anyone has bright ideas for how to proactively address it. If nothing else, at least I'll get "I told you so" rights. ;) (j/k).

Bojhan’s picture

Per webchick's request, some mockups. I have been working on Drupal UI stuff all day, and not too experienced with Drupal.org styles and standards so let me know if its horrible :')

I am using the per style guide "to highlight important information" background on the summary.

Within the issue queue, a comment with the log message and a status change a like "but styled" field. We should probably only apply this color to the last isssue queue summary update.

geerlingguy’s picture

Something along those lines looks pretty reasonable. Maybe a little less emphasis on the 'date last modified', but not much.

dww’s picture

Given how often cross-posting occurs and various meta data changes get clobbered, I imagine a lot of problems with N people trying to edit an issue node simultaneously and all but one of them losing their work. :( that's really a problem in core that we don't have a native write-locking mechanism. I'm assuibg there's a module for that. Split this off to another issue so it doesn't delay initial deployment?

Mockups seem fine, although I wasn't imagining that every edit to the body generates a full issue comment with a number, another email notification, etc. But, it could. However, you can't edit the log message itself, so editing the comment with the copy of it seems wrong. Basically, I don't want to duplicate the data as a comment, just interleave an "activity stream" for the issue, including comments, issue summary edits, eventually Git commits, etc.

Anyway, mostly that 1st screenshot is for the forthcoming issue we need to spin off for #81.d...

webchick’s picture

Update from tonight. I'm going to try one more good go at this sometime this week, and then probably unassign myself for now, because I feel like I'm treading quicksand here. :(

Issue #1: The "revision information" fieldset is stuck at the top of the issue edit form, and no amount of re-weighting in hook_form_alter() gets it to stop doing that. This is an obvious user-facing bug, so I think we can't deploy without fixing this. Dave pointed out that CCK is probably interfering here, but even at the manage fields screen there's no way for me to re-order it properly. Dave suggested turning off JS and manually twiddling the weights, which strikes me as horrifying, but I guess it would work since it's a one-time operation. Any other ideas?

Issue #2: #1178288: Data loss of uploaded files when re-editing issue is not totally fixed yet; dww identified an additional follow-up problem. This is also a deployment-blocker. We also lost a butt-load of time verifying this because of #1182144: Dev site owners cannot create/destroy environments.

Issue #3: I looked into providing tests for said bug, but ended up getting siderailed on SimpleTest's behaviour on staging sites, prompting the creation of a handbook page and two issues. I'm now down to 1 fail and hundreds of exceptions in the Project* tests, which should be enough to start from.

webchick’s picture

Grrrrreetings from #dcco2011!

http://metrics-drupal.redesign.devdrupal.org/ now seems to be working and bug-free. WOOT!! It is running the patches from the following issues:

- #1186042: Implement hook_content_extra_fields() to support re-ordering issue fieldsets to handle the revision information weighting bug.
- #1178288: Data loss of uploaded files when re-editing issue to handle the file/tag data loss on new revision bug.
- #1178296: All file attachments regardless of revision displayed on node view to handle a PIFT bug where issue revisions end up showing N copies of any attached files on the node.
- #1186084: Make issue revisions required to make the revision field required on issues.

Haven't done any UI tweaks yet. Hoping to get time to do that this week. If someone wants to take a stab first, feel free!

Thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you to Dave Reid for coming through BIG TIME on those patches! YAY.

webchick’s picture

Also, going back over the other comments from #73 on...

To-do

  • Change "Description" label to "Issue summary" on node/add/project-issue
  • Come up with a form element description that adequately communicates what the issue summary is for, how to use it, and how to change metadata.
  • Finalize proposed recommended summary template at http://drupal.org/node/1155816 (and add a link to it in said description)
  • Implement http://drupal.org/files/issues/issue-summary-as-node.jpg :
    • Print "last updated" on issue summary
    • Place a prominent "Issue summary" label on node view
    • Change background colour of issue summary on node view

Out of scope

  • Some kind of activity stream thingy that intersperses issue summary edits, commits, etc.
  • Revisit decision to hide all metadata (tags, file uploads, etc.) on issue edit. (I actually like this just fine.)
  • Allow images to be uploaded to issue summaries.
  • WTF @ Documentation input format hiding tab.
  • WTF @ cross-posting saying "someone else already edited this, suckah." (in practice, I don't think this will happen too often; most people comment on, rather than edit, handbook pages for example)
donquixote’s picture

+1, subscribe
(i thought i already was, but apparently not)

jide’s picture

subscribe.

pillarsdotnet’s picture

cweagans’s picture

Assigned: webchick » cweagans

Yoink.

cweagans’s picture

Status: Needs work » Needs review
jide’s picture

I left a comment on the documentation node, but I copy it here in case :

May I suggest that we add a "Current fix" (or "Possible workarounds", something like that) so that end users have a simple solution if it exists ? It would be different from "Proposed resolution" which is aimed at developers.

I come from http://drupal.org/node/228818?page=1#comment-4608452 where a user wanted to patch core although the issue could be easily fixed by enabling CSS aggregation. Issues aim at both developers and end users, and having a section in the summary that gives a fix other than patching or alternatives to avoid the issue (or even patching if that's the only way - but that would be clearly stated) would be useful to them, it happens a lot I think.

neclimdul’s picture

So, this reads a lot better than the IRC discussion but I still feel its the wrong decision. I do have some non-critical comments in the last <p>. The format I've used to break down these concerns is:

  • issue
    paraphrased responses from IRC
    my issues with the solution

Lets break it down.

  • Abuse by newbs.
    We don't have this problem with books.
    Issues aren't books. Different people use them and there are different expectations. As a brand spanking new user I /know/ I'm suppose to interact with issues. Books/doc pages are editable but wow am I really important enough to do that? Lets make sure I know what I'm doing in the forums and issue queue first. Also, I joined to post an issue not edit a documentation page oh and Views sucks!
    We have the node revision
    So, we want our contrib maintainers to take on more work shepherding issues? How many issues do you have waiting in contrib that haven't been committed because the maintainer didn't have time already?
  • Loss of actual original post.
    Node revisions
    So, the only way I can refer back to the actual intent of the original post is to dig through revisions? yuck...
    we could duplicate in the first comment
    Webchick pointed out this would be very hard to migrate old issues to. Also, double comments are going to confuse people.
  • out of order summary.

    Changing the summary mid issue at the top doesn't flow in the thread. It doesn't provide for discussion it just forces an opinion for the thread.

  • Inconsistent with feeds
    We have nice new widgets and feeds for issues but updating the summary updates the original post. This seems to really trash the usability of these feeds.
    We'll see "updated" on the issues and you can click the title instead of the comments
    I don't see that as valid. you loose the #new after you clicked the title and, how do you you know there's a new summary instead of a new comment, etc.

So, the only constructive thing I have to say(and I'm really sorry because I really do understand how important this is to people) is that using the first comment really does seem like a reasonable solution if we can figure something out. It pretty similar to how bugzilla works I believe.

neclimdul’s picture

Also, after talking to webchick, I'm behind doing something more beta-ish and rolling it out for limited use either as core only or as an opt-in per project sort of thing. This addresses the over-arching I don't think this is usable yet concern while meeting webchicks core needed this 3 years ago concern with a compromise.

donquixote’s picture

#106 (neclimdul)
This all refers to the idea of editing the issue node body.
I thought we already had a tendency towards selected comments as summaries?

neclimdul’s picture

If that's true, no one updated me or commented here to that effect. If by selected comments just a flag on the comment then I think that addresses every one of my concerns with node editing and I can think of any problem with it.

yoroy’s picture

Editing the issue node body is the direction we're working with now, not flagging comments as summaries.

jhodgdon’s picture

Because this came up above and on IRC, I just checked into three other open-source ticket systems that I have accounts on. My accounts there have no special privileges that I'm aware of, but they've been open for quite a while, and I've reported issues and participated in discussions in all of them.

1) Bugzilla (as configured at https://bugzilla.kernel.org, the Linux kernel bugzilla)
When you enter an issue, you enter a "summary" (title, in our system) and "description" (node body in our system). The description appears at the top of the comment list, and looks pretty much like a comment, but it's labeled "Description", not "Comment 1". My account there doesn't appear to have permission to edit anything at all on issues (title, status, etc.), just add comments, but I think that someone has permission to edit descriptions.

2) Trac (as configured for the WordPress project, at http://core.trac.wordpress.org)
When you enter a new issue, you enter a summary (title, in our system) and description (node body in our system). The description appears in a yellow box at the top of the issue, along with the issue component, status, etc., and it can be edited (I found at least one issue that had a history, and it said "last edited by", and offered to show me a diff of the changes). My account doesn't appear to have permission to edit issue descriptions, though I do have permission to change issue status and other fields, as well as add comments.

3) Launchpad (as configured for the Ubuntu project, at https://launchpad.net/ubuntu)
When you enter a new issue first you have to do a search -- you enter a one-line "summary" and it tries to find a similar issue, to avoid duplicates, and when you click "no, it doesn't match", it also gives you a page full of help on how to report an issue, and you only enter three fields: component, "summary" (title), and "further information" (our node body). Once you've entered an issue, the "further information" shows up as "bug description" on the page, and I appear to have permission to edit the description on existing issues. There's also a link at the bottom of the description that says "See original description", if it's been edited, and each edit automatically adds a comment to the comment list. The status/component fields are maintained in a block at the top of the issue (and edited there), and it looks like I have permission to edit that information too; editing those fields also adds a comment. All the editing is done via JavaScript buttons (you click a button near the information that is displayed, in order to edit that information).

Summary:
It looks like these three projects have made three different decisions about permissions -- Ubuntu is permissive (anyone can edit description and status fields), WordPress has basically what we have now (anyone can edit status fields but not description), and the Linux Kernel has more restrictive permissions (all I can do is add comments). They all have editable issue description fields, at least by someone, and they all allow anyone with an account to add comments.

Interestingly, they have all called the issue node title "summary" and the issue node body "description". Something to consider. Oh, I guess we call it "Description" too. That's probably good. :)

donquixote’s picture

@yoroy, #110
I lost this last change of direction. Probably because I was focused on the other issue (provide a mechanism for issue meta discussions)
(see #100)

I agree with neclimdul, that making the node body editable like a wiki page is asking for trouble.

If we also disagree with summary comments in the originally proposed sense, then I would have a few different ideas (maybe with the same or similar flaws)
1) Provide a formatting tag "<summary>" similar to "<code>" and "<blockquote>", just that it would mark the enclosed text as a summary. This way, not the entire comment but only that piece of text would be the summary. Again, this probably has the same flaws..
2) Provide a link "create issue summary comment" in a less obvious position than the comment textarea.
3) Let other people flag comments as summary, after they have been created (I think this has been proposed before).

jhodgdon’s picture

#112 - 2 & 3) See above... Using comments has been tried (I made a working implementation), and was killed by the infra team, because of inefficiencies in the Flag module. So we'd have to either rewrite flag or make a specific module for doing this, in order to use comments as summaries. It's not happening right now at least.

#112 - "asking for trouble" - see #111. The Ubuntu project has "anyone edit descriptions" permission enabled, and I don't believe that they have experienced trouble, although they have a huge user base and a huge issue tracking system (presumably if they had experienced trouble, they would have turned this permission off?). We could also quite easily restrict issue node body edit permission to accounts that have some kind of elevated role on d.o, if we're really worried about it, or change that permission to more restrictive if it becomes a problem. Permissions are, as we all know, VERY easy to change in Drupal. IMO, that's not a reason to not try this.

webchick’s picture

The flagging comments as a summary solution is a non-starter, for the following reasons:

1. If I want to make a small correction/clarification to someone's issue summary, I have to re-copy someone else's comment and then paste it again. If someone else wants to make a small clarification to that, they have to re-copy and paste it again as well. This needlessly elongates the issue and makes it more difficult to read and understand.
2. There's also no way to "diff" separate comments to see what was added or changed between revisions. "Wiki" style nodes do not have this problem. You can see exactly what was changed, when, and by whom.
3. And finally, the infra team has nixed the proposal to deploy Flag module for comments due to the unacceptable performance hit (see #60).

webchick’s picture

Thanks for that analysis of other issue trackers, Jennifer!

"There's also a link at the bottom of the description that says "See original description", if it's been edited"

Say, that's a great idea, and would be really easy to tack on to the bottom of the node body. Let's make that a to-do item.

rfay’s picture

And let's keep this going forward. What (almost) happened here is a bikeshed. Coming in late on an issue and suggesting a complete change of direction without understanding the history of the issue is poor form. I hope that's not too harsh. But I don't want this to die.

Now... IF we had an issue summary that explained where we were at on this... maybe it wouldn't be so easy to bikeshed :-) Maybe then you wouldn't have to parse 116 comments before responding. Maybe the issue summary at the top would explain why we weren't doing the comment approach :-)

donquixote’s picture

[off topic]
Hi, to defend myself, I have been active on the other issue before, and only recently noticed it had continued over here.
[/off topic]

webchick’s picture

Another thought on dealing with legacy stuff is to not show the Issue Summary chrome unless the issue was already edited? Not sure.

neclimdul’s picture

Or maybe it was an honest mistake caused by many discussions surrounding this issue that no summary would fix.</frustrated>

@jhodgdon that's great! That's exactly my experience with bug trackers as well. I think many people have been looking at the description from an internal issue tracker where more permissions have been given.

"There's also a link at the bottom of the description that says "See original description", if it's been edited and each edit automatically adds a comment to the comment list." The automatic comment addresses some of my flow of comment concerns too.

cweagans’s picture

re #118: the node body should -always- be the issue summary, right? If I were to go file an issue, I should write an issue summary in the body field and then keep it updated as the issue progresses?

FWIW, I really disagree with adding a comment for every node revision. It would, presumably, be duplicate data - just a copy of the revision message, right? The approach here, from what I understand, is to have a living summary at the top of the issue. I really doubt that the edits will be of that much consequence to most people. The whole idea here is that you don't -have- to see the entire history of the issue in order to grasp what is going on. We want to be sure to make the revisions -available- in case somebody wants to look, but I don't think that we should concern ourselves with adding comments for such activity.

I can foresee situations where an issue summary will get updated 18 times in a row for various reasons and then 300 contributors wake up to 18 emails in their inbox. If we make notification a separate operation that can be triggered by a contributor (rather than an automatic process), we can control the mail spam.

If, at some point, there is a good framework for logging activity related to an issue (issue summary updates, commits, changes in responsibility, etc), that would be a good time to implement the summary update notifications as well.

For now, I think that we should clean up what has been done so far, deploy it, and start using it. At that point, we will be able to identify the parts that need tweaking or reworking.

That is, what is done so far should not be viewed as the last revision. Rather, it should be the first revision of many. This is something that's very easy to change and adapt to our workflow as needed, but we need this stuff available *now*. This will be very important functionality when the Core Initiatives come to fruition.

jhodgdon’s picture

RE #120 - the comment for each edit could just say something like:
jhodgdon edited the issue description
and ideally a link to the diff (which is available if you have node revisions turned on).

RE #120 - YES, let's deploy and tweak if necessary! But there was a short to-do list in #99 that needs to be finished before deployment.

pillarsdotnet’s picture

Does #1098198: Project Issues Summary Proof of Concept relate in any way to this issue? Should I submit a patch to Project Issue implementing the proposed to-do list?

webchick’s picture

The to-do list in #99 seems to be implemented; it's up for review on http://metrics-drupal.redesign.devdrupal.org/node/1061924. (Apache user/pass: drupal/drupal, d.o username/pass: jhodgdon/awesome)

That's what cweagans meant to say in #104. :P

webchick’s picture

Marked #1098198: Project Issues Summary Proof of Concept a duplicate of this issue. We want issue summaries to be as simple as possible so they're not overwhelming to write. The proposed template is at http://drupal.org/node/1155816 as indicated in #99.

See why we need this deployed? :P

webchick’s picture

Issue tags: -Usability, -Accessibility, -prairie

Ok, I've used this issue as a "beta test" for issue summaries by editing the original issue node with a summary of this issue. Hopefully that will help new people coming in.

Unfortunately, only a handful of people with "administer nodes" permissions can change that on d.o as it stands. Once this goes live, though, everyone would have a chance. :)

webchick’s picture

Issue tags: +Usability, +Accessibility, +prairie

Oh right. That bug. ;P That's fixed on the metrics site.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Agreed - everything in #99 has been done. So what do we need to do to get this deployed?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Also, I've adjusted the issue summary template http://drupal.org/node/1155816 to add "Remaining tasks" since that's extremely helpful to know at all times before an issue is actually committed.

This is also where we could mention things like workarounds, per jide's comment. I really don't want to overwhelm people with headings though. This needs to be simple enough that people actually fill it out.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, didn't mean to cross-post. But I think this could definitely still use some more review, particularly from the UX team to ensure their recommendations are implemented to-spec.

webchick’s picture

Issue summary: View changes

Testing out issue summaries on the issue summary issue. How very meta.

geerlingguy’s picture

Issue tags: -Usability, -Accessibility, -prairie

Another note / review of the dev site mentioned in #123 / #104 - I like it. A lot. The only *very* minor quibble I have is that, coming from an 'I've used most of the popular Drupal themes before' background, the background of the summary looks more like an 'unpublished node' than a summary.

But that should not stop this from getting deployed. Just saying that a follow-up might be able to focus on how specifically we should highlight the fact that it's a summary...

geerlingguy’s picture

Issue summary: View changes

Changing "Current state" to "Remaining tasks" and putting a link to the template recommendations.

dww’s picture

Re: #126: Ahh, right. That fix is over at #1178250: Editing a node with existing alterable tags results in data loss - USE #ACCESS! which also needs to be deployed when this goes live. It'd be nice to have a definitive list of every issue that was involved in getting this working so that we don't leave out anything when we're rolling this out on the production servers.

Meanwhile, I'm trying to get all the other dependencies committed and pushed upstream so we can do a safe/sane deployment ASAP.

100 cheers to everyone who's showed up to pour time and energy into this issue, provide patches, and voice their concerns. I disagree that neclimdul was at all guilty of bikeshedding -- that's a misuse of the phrase. Bikeshedding is where everyone has a (strong) opinion about something trivial and unimportant, while really big important decisions are left to a tiny pool of "experts". I don't think anything neclimdul raised qualifies as trivial or unimportant. You might argue he was being an obstructionist, derailing something close to deployment, but I don't think that's right, either. It's far better that he raised his concerns before this was deployed so that they could be heard and addressed than if he showed up a week from now once this was live -- especially if he had concerns we weren't already aware of and trying to deal with...

Anyway, yay for collaboration, and yay for making our collaboration tools better!

Thanks,
-Derek

neclimdul’s picture

Wait, so what solution are we going forward with? Are the auto comments still on the table?

cweagans’s picture

I think the auto comments are being done in a follow-up.

I"ve updated the dev site with some new code.

@#130, I don't much like the bg either and I think we should remove it. it would kill off the changes to bluecheese, as well.

donquixote’s picture

What about instead of auto comments we simply show revisions between in the comments timeline?

webchick’s picture

Summary of a several hour sprint on IRC among cweagans, jhodgdon, sun, yoroy, Bojhan, and some other folks:

- The colouring was happening on the node/comment form as well as the summary. Fixed.
- The description on the body field was inadequate to explain what was going on. Fixed.
- The "Last modified" date didn't quite show enough detail. Now it shows not only who changed it, but however many revisions there've been to an issue and exactly when, to make spotting updates easier. Fixed.
- The issue summary template was difficult to find. Moved it to the top of http://drupal.org/node/1036132. Fixed.
- Concerns about losing the original post were valid. We modified the template to include a heading for "Original report by [username]" for legacy issues (as demonstrated in this very issue!). Fixed.
- We went back and forth between "Issue Summary" and "Description" and whatnot for the title of the body field. In the end, we decided on "Summary", despite the fact that other projects use "Description", because without that this feature is very undiscoverable. Can possibly address more in a follow-up issue, but for now... Fixed.
- The background colour was confusing at least one person, and what we should probably do is revisit the entire design from a more holistic level once we see where the points of confusion are. Removed that for now, to make deployment easier. Postponed.

Other things that are valid concerns, but will need to be follow-up issues:

  • Showing edits in-line in the comment stream. There's a nice API function in Project issue module that's used by "System message" which we could possibly re-use here, but it requires some testing, particularly around what it does to notifications (see #120). Not sure how easy this will be in the sandbox environment. Also, we need a spec for what we want to see happen.
  • Further tweaks to the UI/text. Once this is actually in use, we'll have a much better idea of what people find confusing, and how the system's being used/misused, and can base further designs on actual usage data as opposed to hypotheses.

We can't launch this until these are done and committed:

#1186084: Make issue revisions required
#1178296: All file attachments regardless of revision displayed on node view

dww would also love some tests for :

#1178288: Data loss of uploaded files when re-editing issue

Barring any other major bugs, though, I really think this is ready to roll. SO EXCITED.

cweagans’s picture

There are also code changes to drupalorg_project and bluecheese that need to be included in a commit. Not sure who's responsible for doing such a thing, and I have no idea how to do -anything- with bzr as far as making patches goes, but that's a separate issue.

Code changes are in hook_nodeapi and hook_form_alter in drupalorg_project and at the very bottom of the bluecheese styles/styles.css in the metrics sandbox.

dww’s picture

Status: Reviewed & tested by the community » Needs work

Code changes to drupalorg_project belong as separate issues over at http://drupal.org/project/issues/drupalorg -- patch the 6.x-3.x branch of Git directly -- don't mess with bzr.

Code changes to bluecheese belong as separate issues over at http://drupal.org/project/issues/bluecheese -- bzr diff > filename works fine for generating patches against bluecheese.

In other news, I asked a question over at #1186084-2: Make issue revisions required that'd be nice to get answered. If the answer is "no, that doesn't need to be in the patch", then in addition to tracking/collecting all the various spin-off issues and their patches, we also need a definitive deployment TODO list for all the crap we'll have to do manually via the admin UI on live d.o. I'd rather keep that list empty (if possible) but sometimes the cure is worse than the disease. So, I'm happy to be flexible and open-minded about it depending on what's really on the list. From my memory of this issue, the things on that list would include:

  • Configure project_issue nodes to require new revisions by default.
  • Assign the new 'edit all project issues' permission to the right role(s). (Which role(s) should that be? 'authenticated user'?)

I'm guessing there are more I'm not aware of or forgetting, so I'd appreciate it if the fine folks maintaining the dev site could flesh this out in gory detail based on their experience getting everything working there. Basically, a definitive list of every issue where a patch must be deployed, and everything that's not done in code (and what order, if that matters).

To be really careful here, it'd be nice to spin up a new dev site and have someone else do a trial-run deployment of this just based on those instructions. Then we can test that site to make sure the deployment plan is complete. That's probably overkill, but in the absence of a true staging infrastructure, it'd be a really nice touch. Normally, I'd just do that myself, but I *really* don't have the bandwidth for that right now (alas). If it happens, great. If not, that shouldn't block deployment (although it means we should ensure that the folks who've done the dev site so far are "on call" during and immediately after deployment to help find and fix missing pieces).

However, writing the final deployment plan/instructions is a blocker, which is the only reason I'm setting this back to "needs work".

Thanks again for sprinting on this and getting it so close to the finish line!

Cheers,
-Derek

webchick’s picture

Cool, I definitely hear you on re-doing this from scratch on a new sandbox to make sure we have everything. The code in the metrics sandbox is a bit stale. That should help us get a set of deployment steps together.

cweagans: I just did bzr diff > ~/foo.patch and manually edited it down for only the relevant hunks. Totally sucks, but I'm not sure of a better way. :\

Another manual step I can think of off the top of my head is we had to re-order the various issue fields on the content types page so that the revision message doesn't show smack in the middle of the page. Heh. But I'll work on getting these instructions together (though it definitely won't be tomorrow, so if someone else gets to it sooner, PLEASE do!)

drumm’s picture

I just patch -p4 the BZR diff on top of whatever, like git checkouts. The extra bits fail away and Git can handle what's left.

webchick’s picture

drumm, how do you diff just one part of the tree? That's what I couldn't figure out. So I got a patch with changes to project, project_issue, bluecheese, drupalorg, etc. etc.

dww’s picture

@webchick:
bzr diff sites/all/modules/project_issue
or:
cd sites/all/modules/project_issue; bzr diff .
Basically, if you give it any directory or file arguments, the diff is restricted to those file(s).

webchick’s picture

Huh. Well groovy then! :D Not sure what I was doing wrong. Thanks!

I'll make sure to change this into a "how to roll patches from drupal.org sandboxes" page under http://drupal.org/node/1018084

neclimdul’s picture

Wait there was a sprint? Did I somehow imply I wasn't interest in fixing this?

What happened to the UI review you mentioned webchick? I was interested in discussing it. I'm strongly for Description instead of Issue Summary which is one of of the reasons I wanted to see that discussion...

What does fixed mean for any of your comments?

I you guys are in a hurry especially since we feel like we're in the home stretch but why are we talking about deploying where there are deep concerns with UI and implementation? I'm not talking about just my concerns either.

we should probably do is revisit the entire design from a more holistic level once we see where the points of confusion are." -webchick

Everyone is very motivated and we seem to have a lot of momentum so can't we just calm down and get this right?

Seriously WTF people. I've tried to be accommodating, clear, and rational at ever point in the last couple days but I feel like I'm just getting steamrollered here...

webchick’s picture

I'm sorry for the steamrolling feeling. :( My concern is mainly that discussing further UI tweaks can delay this another several weeks, when this is desperately needed by the docs team, the core team, the infra team, and many, many others, all represented above. So I would much rather base further tweaking on actual evidence of how people actually use this feature, rather than conjecture on how they might use it. The sooner we roll it out, the faster we'll know.

But if you want to work with Bojhan/yoroy and come up with some additional text/design tweaks, we can look at them. This effort is delayed another few days with getting a final patch together, anyway.

However, some of the other (very legitimate) concerns you outlined are simply too large to handle in the initial roll-out. For example, showing edits to the post inline in the comment stream. cweagans raises very valid concerns in #120 about using comments for this. This really needs to be its own, separate issue, and said issue will be much easier to figure out after this gets deployed, so that's why it's kicked to a follow-up.

neclimdul’s picture

Status: Needs work » Needs review

I'm sorry, I missed you citing that as an issue. I thought jhodgdon did a pretty good job of addressing it in #121 so I hadn't responded to it. I will however not that the rest of the concerns about developers waking up to 18 emails the same way I've been responded to about editing the body. Launchpad doesn't seem to have a problem with it.

dww’s picture

Status: Needs review » Needs work

Note: this still needs work since:

A) there are dependent issues and patches (for bluecheese and drupalorg) that need to be created and posted
B) we need a definitive deployment plan/guide/TODO list (presumably up in the summary of this issue)

I'd also love to have all the various follow-up ideas actually posted as their own issues and another section for "Future work" or something up in the summary. That's a good way to get really concrete about our concerns and start getting ready to deal with them in productive ways. But, that's not a blocker for deployment, just a request.

Thanks,
-Derek

merlinofchaos’s picture

Creating a usability problem to fix a usability problem that we have successfully gone 3 years without and are still able to get things done is selfish, short-sighted and wrong. Please do not do this.

webchick’s picture

merlinofchaos: Core *needs* this. Documentation team needs this, too. So do people in all of Drupal.org working on any sort of large-scale initiative. http://groups.drupal.org/prairie-initiative has identified the lack of this functionality as the #1 community performance issue in Drupal, taking time away from absolutely everything else we do: repeating discussions that have already happened, re-reading things others have already read, creating huge barriers to get involved on solving tricky problems.

What specific concerns do you have? How can we address them with simple changes? What others can we address with larger-scale changes, and what do those look like?

webchick’s picture

Issue summary: View changes

Adding a note about other previous failed attempts.

Gerhard Killesreiter’s picture

Somewhere at the top of this, we discussed using flag.module. My comments unfortunately, didn't trigger some patches, or I missed them.

I've created an issue in the flag issue queue: #1204474: Less queries

jhodgdon’s picture

Gerhard Killesreiter: There is already an issue in the Flag module:
#1133956: Improve efficiency when displaying lists of flaggable comments
Yours is probably a duplicate?

xjm’s picture

Subscribe; sorry for noise.

webchick’s picture

Assigned: cweagans » webchick

Yoinking back for deployment steps. I'm spinning up a fresh sandbox now.

webchick’s picture

Also, I spoke with killes and he's on board with rolling out issue bodies as wikis, and then iterating on them with additional features after initial launch. I also heard through the grapevine that cweagans is playing around with various activity stream options, so we could maybe end up with something that gathered not only edits to node bodies, but also commits, etc. oOOOOooOOOOooh.

webchick’s picture

Issue summary: View changes

Same info, less snark. :)

webchick’s picture

Ok, took a first stab at deployment steps http://drupal.org/node/1036132/revisions/view/1551216/1554874 ... still need to figure out cweagans's visual changes and get those into a patch(es).

webchick’s picture

k looks like:

drupalorg_project_form_alter():

+    // Change body label to Issue Summary
+    $form['issue_details']['body']['#title'] = t('Summary');
+    $form['issue_details']['body']['#description'] = t('Learn <a href="!noob_docs">how to report an issue</a>. Use the <a href="!standards">issue summary template</a> to summarize the issue. Others can change this summary. If you make an important change to this summary, add a comment to inform subscribed users.', array('!noob_docs' => '/node/73179', '!standards' => '/node/1155816'));

drupalorg_project_nodeapi():

+
+    if ($node->type == 'project_issue') {
+      // Get most recent version of the node
+      $revision_information = db_fetch_array(db_query('SELECT vid, uid, timestamp from {node_revisions} WHERE nid=%d ORDER BY timestamp DESC LIMIT 1', $node->nid));
+
+      // Get revision count
+      $rev_count = db_fetch_array(db_query('SELECT COUNT(*) from {node_revisions} WHERE nid=%d', $node->nid));
+      $rev_count = $rev_count['COUNT(*)'];
+
+      // Get the revision author
+      $revision_user = theme('username', user_load($revision_information['uid']));
+
+      // Create the timestamp
+      $date = format_date($revision_information['timestamp'], 'custom', 'F j, Y');
+      $time = format_date($revision_information['timestamp'], 'custom', 'g:ia');
+
+      // Replace the issue header label with issue summary and the last updated date
+      $node->content['project_issue_header']['#value'] = '<h2>' . t('Issue Summary') . '</h2>';
+      $node->content['project_issue_header']['#suffix'] = '<p class="project-issue-last-modified">' . t('Revision') . ' ' . $rev_count . ' by ' . $revision_user . ' on ' . $date . ' at ' . $time . '</p>';
+    }

bluecheese/styles/style.css:

+/* Issue Summaries */
+.project-issue-last-modified {
+  margin-bottom: 1.5em;
+}

That's going to need a bit of clean-up work to get deployable. I'll try and get to it this weekend.

webchick’s picture

Status: Needs work » Needs review
FileSize
4.77 KB

Here are some notes and improvements in this patch.

Note #1: Please excuse the long post; trying to impart some mentoring as long as I'm cleaning things up. :)
Note #2: This advice should all be sanity-checked. My mad PHP skillz are frightfully rusty.

  1. Code style fixes. Of course. :D
  2. Numerous t() fixes; Specifically:
    t('...<a href="!noob-docs"> ...', array('!noob-docs' => '/node/73179'))
    

    Changed to (for support with unclean URLs/subdomains):

    t('...<a href="@issue-report"> ...', array('@issue-report' => url('node/73179')))
    

    ! (anything goes) is only needed as a placeholder if we were putting HTML in here, and we're not, so best stick with @ (check_plain).

    And:

    '<p class="project-issue-last-modified">' . t('Revision') . ' ' . $rev_count . ' by ' . $revision_user . ' on ' . $date . ' at ' . $time . '</p>';
    

    This string is not actually translatable. What you'd end up with in French, for example is "Révision 1 by webchick on 2011-whatever". The entire user-facing text needs to be in t(), with placeholders for the variable values.

    We should do this instead:

     '<p class="project-issue-last-modified">' . t('Revision @count by !username on @date', array('@count' => $count, '!username' => $username, '@date' => $date)) . '</p>';
    

    (In this case, ! is appropriate for username because it's going to come back pre-sanitized via l().)

    http://drupal.org/node/322729 has some really good info on proper uses of t(), which is worth a read.

  3. On the topic of dates, rather than doing "$date . ' at ' . $time", go with the same thing Bluecheese is doing for the $submitted variable (see template.php):
          $date = format_date($node->changed, 'custom', 'F j, Y \a\t g:ia');
    

    We were noticing some weird timezone issues when we were testing earlier, and I wonder if that was the cause. It doesn't seem to be happening now.

  4. This patch introduced two queries, which needed to be optimized.

    The first query was doing a filesort (bad!):

    mysql> EXPLAIN SELECT vid, uid, timestamp FROM node_revisions WHERE nid = 8 ORDER BY timestamp DESC LIMIT 1;
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
    | id | select_type | table          | type | possible_keys | key  | key_len | ref   | rows | Extra                       |
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
    |  1 | SIMPLE      | node_revisions | ref  | nid           | nid  | 4       | const |    1 | Using where; Using filesort | 
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
    1 row in set (0.00 sec)
    

    Since we really just need the highest-numbered vid, you can ask for it like so instead:

    mysql> EXPLAIN SELECT MAX(vid), uid, timestamp FROM node_revisions WHERE nid = 8;
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
    | id | select_type | table          | type | possible_keys | key  | key_len | ref   | rows | Extra |
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
    |  1 | SIMPLE      | node_revisions | ref  | nid           | nid  | 4       | const |    1 |       | 
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
    1 row in set (0.00 sec)
    

    However! You don't actually even have to query this because the $node property already gives you:

    $node->vid = the latest revision ID.
    $node->revision_uid = the uid of the person who made it.
    $node->changed = the timestamp of when the node was last changed.

    So you can get all of this info with straight-up node properties that are already available. One query down! :D

    Then this code:

    +      $rev_count = db_fetch_array(db_query('SELECT COUNT(*) from {node_revisions} WHERE nid=%d', $node->nid));
    +      $rev_count = $rev_count['COUNT(*)'];
    

    Can be greatly simplified by:

          $revision_count = db_result(db_query('SELECT COUNT(vid) FROM {node_revisions} WHERE nid = %d', $node->nid));
    

    You can just use db_result() when you only need one value.

    Why COUNT(vid) instead of COUNT(*)? COUNT(*) is optimized for MyISAM storage engines, but Drupal.org is using InnoDB. It's much faster to COUNT on the primary key itself instead of * columns (plus * is just lazy :)). See http://drupal.org/node/224333#select_count for more.

    Here's the EXPLAIN on that query:

    mysql> EXPLAIN SELECT COUNT(vid) FROM node_revisions WHERE nid = 8;
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
    | id | select_type | table          | type | possible_keys | key  | key_len | ref   | rows | Extra |
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
    |  1 | SIMPLE      | node_revisions | ref  | nid           | nid  | 4       | const |    1 |       | 
    +----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
    1 row in set (0.00 sec)
    
    
  5. user_load() in Drupal 6 is actually crazy-expensive, due to lack of caching. It'll end up calling an extra however many queries (and since Drupal.org runs Profile module, it's a lot), when all theme_username() actually needs is $object->uid and $object->name. So I did a direct query for that value instead, and created a mock user object, like so:
          // Make a mock user object for theme_username() to avoid expensive call
          // to user_load().
          $revision_author = new stdClass;
          $revision_author->uid = $node->revision_uid;
          $revision_author->name = db_result(db_query('SELECT name FROM {users} WHERE uid = %d', $revision_author->uid));
          $username = theme('username', $revision_author);
    

    Here's EXPLAIN on that query:

    mysql> EXPLAIN SELECT name FROM users WHERE uid = 1;
    +----+-------------+-------+-------+---------------+---------+---------+-------+------+-------+
    | id | select_type | table | type  | possible_keys | key     | key_len | ref   | rows | Extra |
    +----+-------------+-------+-------+---------------+---------+---------+-------+------+-------+
    |  1 | SIMPLE      | users | const | PRIMARY       | PRIMARY | 4       | const |    1 |       | 
    +----+-------------+-------+-------+---------------+---------+---------+-------+------+-------+
    1 row in set (0.00 sec)
    

Whew! :) Ok, new patch.

Also, new demo site: http://issue-summaries-drupal.redesign.devdrupal.org/node/1206030
(Apache user/pass: drupal/drupal, Drupal user/pass: bananas/bananas)

Please look things over and make sure it behaves as it did on the metrics site (which I'll keep up for another few days just in case).

webchick’s picture

FileSize
2.7 KB

Ahem. And here's one without all the crap from #1186084: Make issue revisions required.

webchick’s picture

Issue summary: View changes

Adding deployment steps.

donquixote’s picture

Something I noticed about the wiki approach.
I just checked my personal dashboard on drupal.org, and found this issue to be "updated". No new comment, however.
Visiting the issue leaves me clueless about what has changed and what hasn't.
The revision diff is a few clicks too far away, so in the usual case I won't go this extra mile.
And once I am on the revision diff, I need to make sense of those changes.

The solution: I think when the step discussed here is complete, we will need to think about ways to make the revision diff more easily accessible, without going to separate pages. Maybe some trick with js-based "tabs" ? (yes this discussion can wait)

webchick’s picture

Did you test the demo site? There's a clear "Revision X by username on date" at the top of the node view, so you can see that it was edited.

geerlingguy’s picture

@160 - I think what he means is it takes too long to actually see what has changed. If someone were to radically change the issue summary, and the issue was relatively old, I'd have to go in and look at the diff to see what it looked like last time I was involved in the issue, then look at what's new in the summary. However, I would hope that people would be discussing changes incrementally in the comments before radically altering the issue summary...

webchick’s picture

FileSize
3.28 KB

Ok, well that's easily fixed. New version on the demo site makes "Revision XXX" a link directly to the diff if the count is > 1. See http://issue-summaries-drupal.redesign.devdrupal.org/node/1206030 for an example.

Here's a new patch.

Though the EXPLAIN on the new query is:

mysql> EXPLAIN SELECT vid FROM node_revisions WHERE nid = 1036132 ORDER BY vid DESC LIMIT 1,1;
+----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
| id | select_type | table          | type | possible_keys | key  | key_len | ref   | rows | Extra                       |
+----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
|  1 | SIMPLE      | node_revisions | ref  | nid           | nid  | 4       | const |    1 | Using where; Using filesort | 
+----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
1 row in set (0.00 sec)

Hrm. :\

webchick’s picture

Issue summary: View changes

Updating remaining tasks and deployment steps.

webchick’s picture

Since #1186084: Make issue revisions required got deployed, I've updated the deployment steps in the issue summary + fixed the visual description. (diff)

geerlingguy’s picture

@162 - That looks good to me, from a usability standpoint. I really think what you said earlier about getting this moving, and seeing how people actually use it, and what complaints people have, will be the most helpful thing at this point :-)

nnewton’s picture

@webchick, That explain looks fine for this. Its not optimal, but that filesort shouldn't hit disk most of the time and if this become an issue, its an easy fix. (index over nid,vid)

The only situation I could see this becoming a problem is if a single node had enough revisions to hit disk hard with this sort. Until we really have a case like that (and if that turns up in testing it would be good to know), I'd be more fine with accepting this "filesort" than an index on node_revisions.

jhodgdon’s picture

I took a look at
http://issue-summaries-drupal.redesign.devdrupal.org/
It looks mostly good... but I had two "WTF" moments:

a) After editing an issue node body (summary), it does not show up in "My issues". The user "bananas" for the test site, in fact, has zero issues in My Issues (and presumably would not be subscribed by email to the issue either). This is kind of a show stopper for me -- I pretty much live on the My Issues page. I realize eventually there are thoughts of making it so that if you do a revision, it automatically adds a comment (and maybe that would fix this problem?), but as things are now, it's kind of broken for me on that basis... I guess I'm supposed to add my own comment saying I've revised the summary... well, I guess we can let this go if we have to, and wait for the new auto-comment feature.

b) When I click "Edit", I can't change the title or any of the other stuff. How about at least having the ability to change the title? It seems as though when I'm making a summary, I might also want to change the title, and maybe even some of the other information? But I guess I have to edit that elsewhere? It seems clumsy to me. Can we at least have the title?

jhodgdon’s picture

I also reviewed the code in the patch in #162. There are minor whitespace issues:

+        // Generate a link to the diff of this revision.
+	$previous_vid = db_result(db_query_range('SELECT vid FROM {node_revisions} WHERE nid = %d ORDER BY vid DESC', $node->nid, 1, 1));
+	$diff_link = url("node/$node->nid/revisions/view/$node->vid/$previous_vid");
+        $modified_text = t('<a href="@diff">Revision @count</a> by !username on @date', array('@diff' => $diff_link, '@count' => $count, '!username' => $username, '@date' => $date));

The left sides of these 4 lines should all line up, and don't.

Other than that minor issue, the code (to me) is clear and passes my review.

jhodgdon’s picture

So, regarding #166, I propose that the last sentence of the help description for the issue's Summary field be changed from:

If you make an important change to this summary, add a comment to inform subscribed users.

to something that conveys that pretty much any change should result in a comment, and that editing doesn't subscribe you...

How about:

Editing this summary does not subscribe you to the issue or notify subscribers. Add a comment describing your changes after any significant edit.

webchick’s picture

FileSize
3.34 KB

#167 and #168 are dealt with in this new patch, and on the demo site.

#166 is unfortunately how it's going to have to work, unless someone can tell me how to get an argument that's "User posted or commented or edited a revision", which doesn't seem to exist. And for now, I think we can consider the lack of this auto-subscribe on edit a feature, given the concerns around transparency to other subscribers when the issue summary is edited.

jhodgdon’s picture

OK by me. I'm ready to say "Let's go with it!". Agreed that the new patch addresses my concerns. :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Tentatively marking RTBC unless someone else wants to object...

webchick’s picture

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

Ok, tentatively tagging this for deployment.

- killes has signed off on the idea.
- Deployment steps are outlined at http://drupal.org/node/1036132#deploy

This patch introduces three queries on issue node pages:

mysql> EXPLAIN SELECT COUNT(vid) FROM node_revisions WHERE nid = 8;
+----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
| id | select_type | table          | type | possible_keys | key  | key_len | ref   | rows | Extra |
+----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
|  1 | SIMPLE      | node_revisions | ref  | nid           | nid  | 4       | const |    1 |       |
+----+-------------+----------------+------+---------------+------+---------+-------+------+-------+
1 row in set (0.00 sec)
mysql> EXPLAIN SELECT name FROM users WHERE uid = 1;
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------+
| id | select_type | table | type  | possible_keys | key     | key_len | ref   | rows | Extra |
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------+
|  1 | SIMPLE      | users | const | PRIMARY       | PRIMARY | 4       | const |    1 |       |
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------+
1 row in set (0.00 sec)
mysql> EXPLAIN SELECT vid FROM node_revisions WHERE nid = 1036132 ORDER BY vid DESC LIMIT 1,1;
+----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
| id | select_type | table          | type | possible_keys | key  | key_len | ref   | rows | Extra                       |
+----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
|  1 | SIMPLE      | node_revisions | ref  | nid           | nid  | 4       | const |    1 | Using where; Using filesort |
+----+-------------+----------------+------+---------------+------+---------+-------+------+-----------------------------+
1 row in set (0.00 sec)

(This query only takes effect for issues with > 1 revision, and was signed-off by nnewton in #165.)

Please Please Please Please Please Please Please Please Please Please Please PLEASE. :)

webchick’s picture

Status: Needs review » Reviewed & tested by the community

x-post

drumm’s picture

For the three module updates, what specific releases or dev versions?

The process for production is http://drupal.org/node/1029362, not drush. Ideally, it should be released versions, but -dev happens, especially on things like project*.

rfay’s picture

Just took a test drive on http://issue-summaries-drupal.redesign.devdrupal.org/ as an ordinary user and it's going to be a big step forward. Thanks to all for the hard work on this.

IMO once we get it actually deployed there will have to be better instructions to a person editing an issue. The current instructions are just the standard bug reporting instructions, and of course when someone is editing an issue it means that there's a lot more information than in a bug-reporting situation.

I think we're going to need some proposed "best practices" to go with this new capability. Like "Mention in the comments when you edit the issue summary". I liked what somebody suggested about copying the original report into a comment (and linking to it from the issue summary/node)

Yay!

webchick’s picture

Ok, created:

#1211312: Please roll a new stable release of content_alter_taxonomy
#1211324: Please create a new stable release of PIFT

Though currently, dev releases of both are deployed to Drupal.org.

Project issue has a long list of things preventing a stable release, so for that one we'd need also need to grab its 6.x-1.x-dev release: http://drupal.org/node/250817

Drupal.org customizations doesn't seem to have any point releases at all. So for that one we need 6.x-3.x-dev: http://drupal.org/node/1019242

I'll update the deployment steps accordingly in a moment here.

webchick’s picture

Issue summary: View changes

Adjusting deployment steps now that http://drupal.org/node/1186084 is committed.

webchick’s picture

Issue summary: View changes

Updating deployment steps, based on Neil's desire to have stable releases.

webchick’s picture

Issue summary: View changes

6.x-2.4 got deployed. HOORAY!

drumm’s picture

Issue tags: -needs drupal.org deployment

I deployed updates to the three supporting modules.

drumm’s picture

Status: Reviewed & tested by the community » Needs work

At least the title in the first hunk should be done by configuration, which will be nicer when upgrading in the future. The description isn't configurable, but please consider the node submission guidelines configuration.

One nitpicky thing is I like to use === for things like $node->type == 'project_issue'. Doesn't matter a whole lot.

Otherwise, looks straightforward enough.

webchick’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

Hm. Unfortunately, the body field re-title has to be done in code because this hunk in project_issue module hard-codes it:

  $form['issue_details']['body'] = array(
    '#type' => 'textarea',
    '#title' => t('Description'),
    '#default_value' => $node->body,
    '#rows' => 10,
    '#required' => TRUE,
  );
  $form['issue_details']['format'] = filter_form($node->format);

So I can see two options:

1) Initially launch with the field called "Description", and look into changing it at a later time, if the label is actually found to confuse people in the wild (we're speculating atm, though I think it's reasonable speculation).
2) Patch project_issue module to get it to respect the settings there by calling node_body_field() instead like Blog module does. Not sure what kind of upgrade implications this entails. Created issue at #1211500: Body and Title field labels are hard-coded.

I'm leaning towards 1) and re-wording the help text slightly instead to reflect this.

On http://issue-summaries-drupal.redesign.devdrupal.org/node/add/project-is... you can see the help text as submission guidelines instead, and the body field re-titling gone. Patch updated to remove those elements (and the === stuff, though that seems a bit unnecessarily cautious :)). The nice thing about deploying it this way is that we can make further tweaks to the help text (which will almost certainly be required) in the UI rather than doing additional code deployments. The downside is the help text is physically located pretty far away from the actual body field. OTOH, it's BIG AND BLUE which might actually work to draw more attention to it and see it actually be read. OTOOH, that might make neclimdul and merlinofchaos hate it even more. :\

Putting back to needs review to get some eyeballs on this, but once again I would love to deploy this as-is and handle further UI tweaks after we find out how users actually interact with this functionality in the "real" world for a couple of weeks.

webchick’s picture

Issue summary: View changes

typos.

webchick’s picture

Ok, well #1211500: Body and Title field labels are hard-coded now has a patch for review, at any rate.

webchick’s picture

Issue summary: View changes

Updating deployment steps and UI changes once again.

webchick’s picture

I updated the deployment steps to remove re-weighting of the issue fields; greggles noticed today that the fields were in a weird order, so I assume project_issue module was updated sometime in the past few days. Fixed.

Once again, what's remaining to get this RTBC is:

- A code review of the patch at #179 (it's the same patch without the hard-coded text changes)
- A UI sign off of this: https://img.skitch.com/20110712-1rck17i3reajb5cb9jtfgg633d.png

Any help would be greatly appreciated. :)

webchick’s picture

Issue summary: View changes

Removing several deployment steps that have already been done.

jhodgdon’s picture

CODE REVIEW

I have done a careful look at the code in the patch in #179. It is very clear, and well-commented. The only thing I saw that could be changed is here:

+      else {
+        // Same text, no link.
+        $modified_text = t('Revision @count by !username on @date', array('@count' => $count, '!username' => $username, '@date' => $date));
+      }

Since $count is always 1 for this block, this could be:

+      else {
+        // Same text, no link.
+        $modified_text = t('Revision 1 by !username on @date', array('!username' => $username, '@date' => $date));
+      }

Not a big deal, though.

UI REVIEW

Since you've moved the text to the top, it seems a bit confusing -- I would change "this summary" to "the summary", since the text is not appearing right below the Description/summary field any more and "this" confused me... So the last two sentences would change to:

Others can also change the summary. Editing the summary does not subscribe you to the issue or notify subscribers; add a comment describing your changes after any significant edit.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Anyway, since that UI bit is not in the code, I'd say this is ready to deploy!

yoroy’s picture

Jhodgon's suggestion for the submission guideline makes sense. If you're changing this before deploy I would make that

Others can also change the summary. Editing the summary does not subscribe you to the issue or notify subscribers *so* add a comment describing your changes after any significant edit.

(replace the semi-colon with "so" to relate the two statements a bit more)

Otherwise, go for it!

jhodgdon’s picture

If you're using "so" (which I think is fine), put a comma before it [it is a conjunction joining two sentences, so needs a comma, for all you grammar/punctuation nerds out there]:

Others can also change the summary. Editing the summary does not subscribe you to the issue or notify subscribers, so add a comment describing your changes after any significant edit.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.08 KB

New patch, addressing #182.

webchick’s picture

Issue summary: View changes

more deployment steps tweaking.

webchick’s picture

FileSize
2.47 KB

Oops. Forgot that chunk from bluecheese.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

New patch addresses earlier (minor) issues. I think this is RTBC now. I also edited the issue summary section about deployment so that it has the new suggested text from #185, and correct permission instructions.

jhodgdon’s picture

Issue tags: +needs drupal.org deployment

tagging

webchick’s picture

YAY!!! :D

Re-tagging.

drumm’s picture

I changed the added margin in bluecheese to be 1.385em, which works out to 18px, our leading in paragraphs. See http://webtypography.net/Rhythm_and_Proportion/Vertical_Motion/2.2.2/ for the theory (yes getting this working everywhere is futile, but I try.) Going ahead and committing that part, so I only have one CSS-changing deployment today.

klonos’s picture

Don't forget to let people know about this change once deployed. Those not following in this issue here might get a big surprise, so I think a proper announcement in the front page would be great.

Other than that... YAY too!!!

drumm’s picture

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

Deployed

donquixote’s picture

nice, nice.
Now I guess this needs a follow-up:
- do we need to make the revisions even more visible and rapidly accessible? how can we visualize recent changes?
- what can we do if the summary gets painfully long? We are used to long issue starters, but usually you have to read that only once. The summary wants to be read again every time it changes.

Should we discuss this stuff here, or somewhere else?

geerlingguy’s picture

I'm noticing a much narrower column of content now in Chrome. Just FYI. Might not be fixed, per se... The Change Records for this Issue box is basically taking the entire right column. I presume I'm not the only one seeing that issue...

drumm’s picture

The narrower column is actually from #648218: Make API changes in Drupal core be nodes. It should actually be only showing when relevant, see #1217118: [meta] Followups for API change nodes. I think the narrower column is not bad.

- Wide columns are harder to read. It should be easier for your eyes to get to the start of the next line now.
- More stuff can be hung there, I've noticed people working on related issues.
- And I would like to see the status change boxes moved out there, or other rearrangement to make good use of space.

But, those are all separate issues.

geerlingguy’s picture

Wide columns are harder to read. It should be easier for your eyes to get to the start of the next line now.

+1 there - the new d.o redesign was a bit jarring, and I kinda like these columns more. Just mentioning the fact :)

webchick’s picture

Category: feature » bug
Priority: Critical » Major
Status: Fixed » Needs work

First of all, OMG YAYYYYYYYYYYY!!! :D :D THANK YOU THANK YOU THANK YOU

Second of all, something's wrong with that "revision X by FOO" line.

It looks like $node->revision_uid is the uid of the last person to make a comment on an issue, not the person who last edited the node (wtf?).

Working on another query now...

webchick’s picture

http://issue-summaries-drupal.redesign.devdrupal.org/node/1179994 is an example on the staging site, btw. The node was created by lvto2000, but last commented on by GloGi.

The text says "Revision 1 by GloGi on July 1, 2011 at 7:43am"

Should say: "Revision 1 by lvto2000 on July 1, 2011 at 7:43am"

webchick’s picture

silverwing’s picture

Is deleting comments a reason for a revision by the deleter to be noted?

I deleted spam from #1217224: Full page Layout and there's a revision entry, which doesn't make a lot of sense to me.

xjm’s picture

BUNNIES.

xjm’s picture

Issue summary: View changes

Fix items 2 & 3 in deployment

Jeff Burnz’s picture

This is great work people, very much appreciated for all the efforts going into this.

Revision 16 by webchick on July 13, 2011 at 8:05am

This is excellent - being able to compare two revisions, very easy and makes sense. If I need more I can view the Revisions tab - very normal flow for anyone used to the Handbooks or Drupal revisions in general - and not hard to get the hang of.

What seems to stand out as superfluous is the block "Change records for this issue". Since this is not working correctly at the moment I assume this will show a list of revisions - this seems like something that does not need to be exposed immediately since we already have two other ways to retrieving this information.

The block is also introducing a new term "Change records" which is actually the same thing as "Revisions" (afaik). This could be quite confusing (are they revisions or something else I don't know about?).

How about runing without this block to begin with and see how things go, right now its quite distracting and not really giving me anything I can't get with one or two clicks if I really need that information.

klonos’s picture

We already have a mechanism to directly jump to new posts when one clicks on the "updated" link of an issue. Why not provide a "new" yellow tag in a form of a link that directly shows a diff between version of the summary when the user last visited and the updated one?

Do I make sense?

webchick’s picture

Once again, that new sidebar block has nothing to do with this poor, innocent issue. :) That was introduced in #648218: Make API changes in Drupal core be nodes which has a follow-up issue in #1217118: [meta] Followups for API change nodes to deal with this and other issues that popped up after deployment.

webchick’s picture

"Why not provide a "new" yellow tag in a form of a link that directly shows a diff between version of the summary when the user last visited and the updated one?"

Unfortunately, this doesn't work. As soon as you view the node, the "new" flag gets reset. This is a behaviour of core. :(

I created as new issue at #1217646: Tweak UI of issue summaries for discussing further UI tweaks, since this issue is long enough. :P

jhodgdon’s picture

webchick: Can I suggest that we work on follow-ups to this issue on a separate issue? This issue has gotten too big.

jhodgdon’s picture

Issue summary: View changes

Removing (now duplicate) title.

sun’s picture

Also separate issue, but I'm in a hurry, so leaving the note here:

mmm, I guess input format access permissions will get in our way very soon.

To remedy that, we need to allow to use IMG + TABLE for Filtered HTML; or more generally, all the stuff that's possible with the Documentation input format.

pillarsdotnet’s picture

@#203:

The block is also introducing a new term "Change records" ... are they revisions or something else I don't know about?

I believe the "Change records for this issue" block is a result of #648218: Make API changes in Drupal core be nodes

See working examples at #237634: Rename node_access_write_grants() to _node_access_write_grants() and discourage its use
and #1096446: entity_label() is not passing along the $entity_type parameter

sun’s picture

Another follow-up:

The "Revision X" link points to a diff, but the node IDs are reversed; i.e., in the revision diff, the new version appears as old, the old appears as new.

Example: #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent.

DamienMcKenna’s picture

Am confused on how this is supposed to work - I modified the original node for an issue but the "Change records for this issue" block still says "Invalid project or no changes found"?

xjm’s picture

Am confused on how this is supposed to work - I modified the original node for an issue but the "Change records for this issue" block still says "Invalid project or no changes found"?

That sidebar block is from a separate change. See the Revisions tab (and note the last attribution is currently incorrect because of the issue above).

xjm’s picture

Issue summary: View changes

Updating issue summary to reflect current remaining tasks, + follow up issues.

pillarsdotnet’s picture

Issue summary: View changes

Responding to multiple people mistakenly thinking that the "Change records for this issue" sidebar relates to this issue.

jhodgdon’s picture

Status: Needs work » Fixed

OK...

I have moved all of the follow-up comments to the new issue webchick created:
#1217646: Tweak UI of issue summaries

As for the sidebar block, as noted above it's being handled on
#1217118: [meta] Followups for API change nodes

Setting this issue back to Fixed. Please add new comments on 1217646 instead. Thanks!

jhodgdon’s picture

Issue summary: View changes

Inserted a line break for clarity.

arlinsandbulte’s picture

Status: Fixed » Active
FileSize
57.58 KB

Is this working correctly?
If I add a comment to an issue started by someone else, "Issue Summary" says that _I_ edited the issue when I did not. I only added a comment. (see attached screenshot - #1219446: Is the new branch of 3.x the one to start testing or is that not working?)

Is that intended?

arlinsandbulte’s picture

Status: Active » Fixed

Ah... noticed this issue mentioned in #200, which is the problem I noticed.
#1217190: Interaction between project issue's comment behavior and node_save is causing revision author to update to comment author

Setting back to fixed...

donquixote’s picture

(see #106)
> we could duplicate in the first comment
> Webchick pointed out this would be very hard to migrate old issues to. Also, double comments are going to confuse people.

I created some issue summaries myself recently.
I always had the idea it would be wise not to kill the original post, so I mostly did put the summary on top.
I think we need clear guidelines for this (but only after we have discovered what is the most useful thing to do).

For me it feels the most logical to see an up-to-date wiki summary on top, and below that the original post - to not let the first comments get out of context.
This original post can be made to look like a comment, even if it is not actually a comment (but the node body).

The alternative (as we do it now): Wiki-like edit access on the original body, with some guidelines about writing issue summaries.
The benefit is that it allows inline annotations in the original text.

donquixote’s picture

Here are some example issues:
#653784: Separate out menu links from hook_menu (short "battle plan" summary, big original text)
#512962: Optimize menu_router_build() / _menu_router_save() (big summary, short original text)

xjm’s picture

#217: The summaries helped me understand #512962: Optimize menu_router_build() / _menu_router_save() well enough to review the patch. :) However, there is a standard template (should be linked in the blue box when you edit the issue): http://drupal.org/node/1155816

jhodgdon’s picture

If you have follow-up suggestions, PLEASE do not add them here. This issue is fixed. Follow-ups are on:
#1217646: Tweak UI of issue summaries

joachim’s picture

I think the implementation here needs tweaking: see #1221190: The Issue summary should augment the original post, not replace it.

Status: Fixed » Closed (fixed)

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

dww’s picture

Issue tags: +prairie

Not sure why this tag went away, but this was an effort that was relevant to the Prairie Initiative...

dww’s picture

Issue summary: View changes

adding UX requirement established earlier

  • Commit b6f0512 on 6.x-3.x, 7.x-3.x-dev authored by webchick, committed by drumm:
    #1036132 Provide a mechanism for issue summaries