make project_issue.module use comment.module.

especially given the last paragraph of this bug report, it sounds like the issue is being being saved without having properly loaded the node compleetly. thus the loss of node_access info and upload info. just a guess though.

if i had to guess, i'd say that upload_nodeapi('update') is mistakenly deleting from the files table upon followup. you could verify this by installing devel.module and scanning the query log.

I was having a similar problem in a module that I've been writing. For me the problem was caused because i wasn't calling node_validate before I saved the node.

Here's the current code that I use to load and save a node, attachment listings seem to be preserved properly.

$node = node_load(array('nid' => $nid));
$node->taxonomy = array_keys(taxonomy_node_get_terms($nid));
$node = node_validate($node); // THE FIX
node_save($node);

I'm not sure that the second line ($node->taxonomy) is necessary, but I remember reading somewhere on the drupal.org that it is. Does anyone know if that's true?

Thanks,
Jesse

For the time being, project module is apparently not compatible with upload.module. Of course, project.module should rather be rewritten to use upload module instead its custom solution. I change the title and make it a feature request.

I've been usign project and node privacy by role successfully.

actually, comment.module plus comment_upload feature is whats needed

I've been looking some ways to migrate issue comments and comment file attachments to comment.module and upload.module.

Current Implementation

Note that this is the implementation which is likely used on drupal.org. The project.mysql in cvs HEAD differs as I will discuss further below.

Issue comments are currently stored in the project_comments table.

CREATE TABLE `project_comments` (
  `cid` int(10) unsigned NOT NULL default '0',
  `nid` int(10) unsigned NOT NULL default '0',
  `uid` int(10) unsigned NOT NULL default '0',
  `name` varchar(255) NOT NULL default '',
  `created` int(10) unsigned NOT NULL default '0',
  `changed` int(10) unsigned NOT NULL default '0',
  `body` blob,
  `data` blob,
  `file_path` varchar(255) NOT NULL default '',
  `file_mime` varchar(255) NOT NULL default '',
  `file_size` int(11) NOT NULL default '0',
  PRIMARY KEY  (`cid`),
  KEY `project_comments_nid` (`nid`)
)

Comment body text is stored in binary/blob format for some reason (insufficient or missing filtering support at the time?). The data field seems to store descriptions of the issue state changes (I only quickly looked at this so far). Issue comment ids would collide with regular comment ids.

Files may be attached to both issues and issue comments, though only a single file per issue or comment is supported.

Issue comment file attachment metadata is stored in the project_comments table and the files are saved to the "files/issues" directory.

Proposed Changes

First, eliminate comment file attachments and add them all to the issue node. When a user submits a followup/comment, a custom file upload form (or the standard upload form if possible) but the file will be attached to the issue node. The "attachment: blah.patch" node could/would still appear in the
comment along with any other status changes but all attached files would be displayed under the issue (as it should be in my opinion).

Second, let comment.module handle followups/comments. Issue status changes could be handled in a number of ways. The status change messages could simply be prepended to the comment body text or they could be added to a separate issue status tracking table.

Optionally, an extra file metadata table may be needed down the road to store
file attachment status (e.g. obsolete patch).

As I mentioned, the tables defined in cvs HEAD differ from above. It looks like effort has been (or will be?) made to allow upload.module handle uploads but still attach them to comments. This will not solve the problem discussed in http://drupal.org/node/13221 though and does not allow us to use comment.module.

I encourage feedback on this proposal, especially where any of it is not currently possible. I will need to make myself much more familiar with project.module before I could say definitively if it's all doable.

I think you're largely on the right track here, tangent.

I suggest that we use a combination of comment.module and some custom handling.

• form is constructed by comment.module and then altered by comment.inc to add the needed fields (adapt from the existing form code in comment.inc)
• data are stored in a combination of the comments table and the existing project_comments table, which is revised to have a cid related one to one with comments.cid.

Probably the issue changes should be done as revisions to the issue node. Then we wouldn't need a project_comments table. We would handle node-related data (changes in issue status, etc.) through node revisions, including file uploads. Just the actual comment would be in the comment table. See comments on this (now closed) this (now closed) issue.

nedjo: i don't fully understand your proposal regarding node_revisions. given that the node_revisions table doesn't have fields for all the stuff we want (e.g. status, component, etc), i don't see how we can store such changes there. are you talking about serializing this info and stashing it in the teaser or something? that seems kinda wonky. i'm still totally in favor of this effort, but i'd like to have a clearer sense of the plan.

maybe what we need is to add a vid (version id) field to the {project_issues} table, and having it then behave like {node_revisions}. so, each comment added to an issue that changes anything about the issue itself creates a new record in {project_issues} with a new vid. then, we'd either need to store this vid with the comment (don't see how we could do that directly in the system-wide {comments} table), or keep a {project_comments} table that only needs 2 columns (cid, vid) to hold this mapping.

i suppose we could use the cid itself as the new "vid" in the {project_issues} table, but that would require that every comment creates a new revision, even if it didn't change anything about the issue (e.g. it only adds more text). so, while i know we'd have an extra join with the separate table, we'd reduce duplicate data in the DB. not sure which is the more important thing to optimize, but i'm leaning towards having the right schema and then use indexes and properly written queries to keep things fast. (re-reading previous comments, i guess this is exactly what nedjo proposed in #12 -- i guess i'm in support of #12 but confused by #13).

ideas, thoughts? this isn't the top of my plate just yet, so i'm not ready to assign this issue to myself, but i'd like to start coming up with a detailed plan of attack in the background while i work on other things.

thanks!
-derek

wow, http://drupal.org/node/74995 was a *fantastic* learning experience for both how to do schema refactoring and DB update stuff in general, and to more completely understand the project codebase...

once that patch is commited, there's a real project node for project_issue.module, and we commit to going that route, i think it will only take me another few hours to generate a patch for this issue. if you care about this, please review #74995 so i can move forward. thanks.

this issue predates the split from project into project_issue, but it's really exclusively about project_issue + followups. i keep forgetting to look in the wrong queue, so i'm moving it to the right place. ;) oh, and i do hope to work on this at some point during the 5.x development...

FYI, casetracker ha s apretty nifty implementation of followups as comments. the comments can make changes in the node, upload files, etc.

yup, i planned to "borrow" heavily from casetracker when i finally sit down to work on this (unless i can convince morbus to work on it first). ;)

My understanding is that you want to use the drupal comment module within the issue tracker.
I have a question.
Will the comments be threaded (e.g. like in the forum), or flat (like currently, here)?
I am having a hard time imagining issues with threaded comments. Where is the last comment that need review? Where is the last patch?

Maybe an option will be / should be added to comment.module to force the comments to be flat for certain node types?

I don't recall having seen this discussed (flat/threaded comments in issue tracker), so I thought I'd bring this up here.

http://drupal.org/node/56292

CREATE TABLE `project_issue_comment` (
`nid` int(11) default NULL,
`cid` int(11) default NULL,
`prid` int(11) default NULL,
`rid` int(11) default NULL,
`component` varchar(255) default NULL,
`category` varchar(255) default NULL,
`priority` int(11) default NULL,
`assigned` int(11) default NULL,
`sid` int(11) default NULL
)

if we can get this patch done, i am hoping that replies.module can help with sending email notifications. - http://drupal.org/node/146291

I am working on this currently.

My understanding is that you want to use the drupal comment module within the issue tracker.

I have a question:

Will the comments be threaded (e.g. like in the forum), or flat (like currently, here)?
I am having a hard time imagining issues with threaded comments. Where is the last comment that need review? Where is the last patch?
Maybe an option will be / should be added to comment.module to force the comments to be flat for certain node types?
I don't recall having seen this discussed (flat/threaded comments in issue tracker), so I thought I'd bring this up here.

@beginner: You asked the *exact* question in #19 above, and I answered in #20... Wow, and I thought my memory was bad. ;)

Ouch! Sorry for wasting your time.
I hadn't realized that the link was in reply to my question.

But it looks like that feature won't be in before Drupal 7 at the earliest, while this issue is set against 5.x-0.x-dev.

(dww, for different reasons, I owe you a few good patch reviews! I'll try to find some time for project*.)

Against DRUPAL-5--0-2-BETA . You should be able to use comments to follow up to issues. Hint: use the drupalorg_testing install profile, makes your life so much easier. Next step: update path. And a core patch to set threading by content type.

Note that .install is untested. if it's $%^&U then the table structure is in #22 just use pid not prid.

More notes: without http://drupal.org/node/163029 title won't change appropriately. If you do use drupalorg_testing profile, change comment settings: threading to flat expanded and oldest first. Core patch is forthcoming which will set comments settings per content type (this will get into D6 and into the D5 on drupal.org whether it can get into the mainline D5 does not belong here, I already asked the powers that be, stay tuned).

subscribe ;)

@chx: you rock. ;) re: core patch for threaded vs. non-threaded comments: http://drupal.org/node/56292

there is some mixup with the patch which causes (cvs vars bleh) it to fail on comment.inc . As that's a complete rewrite, there is very little point actually providing a patch for it... so here it is.

chx: i notice the patch includes a bunch of hunks that only fix minor whitespace bugs. as per our discussion on IRC, i'd rather commit those directly and separately, since i'd like to keep this patch just focused on the task at hand so that it's only dealing with followups-as-comments, not whitespace. see http://drupal.org/node/163102. thanks.

Patch against HEAD which is according to dww is where the Drupal 5 development is -- and contains the whitespace fixes.

This is extremely exciting and great. So much potential here...

Minor re-roll to rip out project_issue_link() -- we don't want to be adding those "Follow up" links to issue nodes anymore.

Here are the other problems I've seen so far:

A) Previewing a followup loses the feature where any issue metadata you're changing is previewed as well (try it on d.o to see what I mean).

B) Previewing a followup also generates some weirdness at the bottom of the page:

comment #107
body of comment #107

(Screenshot coming in another followup). Instead, this should include the issue and all existing followups.

C) Even with http://drupal.org/node/163029 applied, issue followups that change the title of the issue don't change the title of the issue node (again, screenshot coming later).

D) The UI of the "Subject" field is rather confusing, given how people are used to dealing with comment subjects. I'm sure you spent a little effort trying to replicate the existing behavior as closely as possible, but since we're going to make this change anyway, I think we should use it as a chance to improve things even more. So, I'd rather there was just a separate "Title" field with a description about "This is the title of the issue itself" and give comments a chance to modify it, but still provide a subject for the comments, since that's what everyone who comments in forums is used to and will expect.

E) The "Your name:" part wastes a lot of space on the followup form. Can we form_alter() that away in this case?

F) Using the "Comment viewing options" feature in core (and toggling the site-wide default setting), it doesn't seem possible to view followups threaded, even if you wanted to.

G) The existing issue followups let you specify a blank body so long as you change something else (title, metadata, etc). Here, the "Comment" field is always required.

H) The project_issue.js code to alter the project-specific metadata on the fly when you change the Project dropdown is no longer working.

I) What's the deal with attaching files to followups? Is there another contrib for that I should be reviewing/auditing/testing? Don't see any reference to that in this issue so far, other than #11 which is clearly stale.

J) A little more phpdoc would be nice. For example, a comment to clarify what the $additions variable in project_issue_comment_view() is for (after reading the code briefly this morning, I think I get it, but I might not get it right at 3am (when I do most of my project* hacking)). ;) This is not a hard requirement before committing, and I might comment stuff myself if it's not sufficient for my liking, but I wanted to mention it here. As you're working on other iterations of this patch, feel free to add more comments.

I haven't closely reviewed the code -- most of this is just functional testing. I'll save my careful review time for a later version that addresses more of these points.

Thanks so much!
-Derek

p.s. I asked chx in IRC what "iaf" was supposed to mean from his patch filenames. Apparently, it was a late-night typo. He was aiming for "issue followups as comments", ifac, which is what I'm now using for attachments for this issue. ;)

Screenshot for #36.B

screenshot for #36.C

Preview weirdness, yes. title changes see screenshot. uploads are handled by comment_upload, maintained by Heine -- I do not think that needs much security audit :)

Given 36.D, it's not clear that we should spend too much time trying to figure out what's going on with the current title stuff. However, I don't know what to tell you. ;) I applied both of your patches and generated the screenshots. Can't see how caching could having anything to do with this. Perhaps your local copy includes other changes that aren't in either patch? Anyway, I'll just stay tuned for another patch to address more of the stuff in #36, and I'll take a look at comment_upload. Agreed that it probably doesn't need an audit. ;)

There is still an issue -- comment preview does not show the project_mail_summary table. But, it seems good to me. Strange preview artefact was caused by collision of project_issue pid and comment pid, I tried to fix with a #tree TRUE, hope it works. Compared to last patch, project_issue_comment_view is much more simpler and faster.

From Derek's big review the following issues remain:
A) Previewing a followup loses the feature where any issue metadata you're changing is previewed as well (try it on d.o to see what I mean).

F) Using the "Comment viewing options" feature in core (and toggling the site-wide default setting), it doesn't seem possible to view followups threaded, even if you wanted to. (hmmm, not sure, it's now maybe fixed)

H) The project_issue.js code to alter the project-specific metadata on the fly when you change the Project dropdown is no longer working. (JS is not my cup of tea)

J) A little more phpdoc would be nice. (Without a doubt...)

The attached patch compared to the previous fixes E) by unsetting $form['_author']

Only the JS issue remains: I refactored my code, so now preview is possible and added tons of doxygen. Also, verified that threaded works, this comes from the fact that we now precompute the metadata tables -- one query is enough.

pwolanin had a freakin' brilliant idea... we discussed that threading just won't work because the metadata does not yield too well, what if a patch is in "needs work" state and the other "needs review" and he pointed out that _comment_get_display_setting uses GET to set the mode and the sort. So I added four lines to project_issue_menu and now we have oldest first, flat expanded.

cvs surprises me sometimes... this time it shipped an empty patch.

generally this is looking good, though I think setting the comment settings via $_GET should be in function project_issue_issue_nodeapi().

this is just code review, no testing...

patch applies cleanly.

• in the form case of project_issue_comment, perhaps a code comment about the way the form is built. looks like we're borrowing another form and modifying?
• for the validate case in project_issue_comment, looks like there are some stray test vars? also, that array_diff line there could use a code comment, since it's a bit confusing to grok.
• in the insert/update case of project_issue_comment, it looks like we should be using a break there, not a return, i think
• i'd like to see a code note for what project_issue_form_alter is doing
• project_mail_notify simply returns nothing? why is that function there if its not doing anything?

for the validate case in project_issue_comment, looks like there are some stray test vars? -- no, actually the array_diff needs to use $test and not $arg , good catch.

project_mail_notify used old comment code which is not there and I am not going to port that, subscriptions will take over. I am willing to write the subscriptions integration code.

I'm all for re-using code and making project not re-invent the wheel so much. However...

A) Who's going to wage the battle to convince Dries + Gerhard to allow subscriptions.module on d.o?

B) I don't think subscriptions is up to the task for stuff like "Subscribe to all of my issues in all projects", or "subscribe to all issues for project foo". (I don't know for sure, I've never actually looked at or used subscriptions.module).

C) Of all the crappy parts of project*, mail.inc is not the worst.

D) mail.inc also holds the mailhandler hooks. Those absolutely have to stay, since that's a critical feature I have to deploy for the project* site at my day job in the near future.

Assuming there are good answers to these (and there probably are), I'm happy to proceed down this path. But, I wanted to raise these now so there are no surprises later on.

Thanks!
-Derek

I will take Dries. Subscriptions 2.x is being written now (by yours truly) so it's up to whatever we want to be up to. The feature to subscribe to a specific node type is definitely on the drawing board. To get you subscribed to the node you have commented upon -- that can't be hard either. mail.inc can stay, just the notification parts needs to be ripped.

Great, that all sounds like good answers to me. ;)

One lingering thought -- what about per-project subscriptions? Maybe we need hook_subscriptions or something, so that project_issue has a chance to do the project-specific stuff? Dunno exactly how best to handle it, but clearly we have to maintain this functionality, since *many* devs rely on it.

Anyway, my much better concern is the upgrade path, so let's focus on that next, and we can worry about subscriptions later.

Thanks!
-Derek

I addressed hunmonk's concerns. No real need re-review because it's just comments (plus using $test in validate). Now, the update path... laters. This week, I hope.

@chx: I'd love your thoughts on http://drupal.org/node/140473 which is about how we display the "diff" generated by any given followup... It's obviously related to this effort. Thanks.

bump ...any updates on upgrade path? lets keep this one moving along ..

Keepin' up w/ HEAD.

gave this code a pretty good workover. couple of things needed to be fixed:

• postgres syntax for the table creation was wrong -- postgres doesn't use 'unsigned' for int. also made cid a SERIAL.
• i've removed the validation check that threw an error when a followup was submitted that had no changes made. this would be sloppy to implement given that we're now depending on other modules (comment/upload/comment_upload) to generate changeable data fields. there was nothing wrong w/ that validation check, but i don't really see it as necessary.
• the metadata change table needs to bypass version data comparison if project_release isn't enabled, or if a project has no releases. this was most easily accomplished by inserting a dummy value of 0 for rid in these cases, which matches what's being pulled from the db.

attached patch addresses all the above.

i'd still like to see a bit more testing, and i'm going to give the code one more run through, but i think this is very close.

attached also includes the initial stab at an upgrade path, and adds dependencies for comment/comment upload. i'm pretty sure we'll want to remove the dependency on comment upload for D6, but it's there for now.

the feature itself is working very well from my testing -- at this point the only thing i can see that needs work is the theming aspect.

the upgrade path still needs work. while the followups and files are properly updated, the metadata changes between followups are being corrupted. everything ends up in the right place, but the path to get there is borked.

attached fixes the metadata change mapping bugs. however, we have another problem. the metadata tables for followups are generated by comparing the last followup metadata values to the current followup's metadata values. however, in the case of the first followup, there's nothing to compare it against.

chx orginally solved this problem by leaving the original issue metadata values in the db, instead of updating them to the most current, and the main issue metadata table contains the metadata from the original issue post, instead of the most up-to-date metadata (which is the current UI). i don't like this solution, because the most current metadata really should be available at the top of the page like it is now.

available options include:

• store both the original and current metadata with the project issue, and use the original when generating the metadata table for the first followup.
• generate the current metadata table dynamically as each issue followup is built (ie, start with the original values, and update them as they are updated in each followup).

i think both have advantages and disadvantages. or maybe there's some other way which i haven't thought of...

Re: metadata for the initial post and related woes... please see http://drupal.org/node/151984

Nice work moving this along. No time now for a careful review -- still digging out from the mountain of email and issue replies that piled up while i was on vacation...

I'm liking the work on ifac. Subscribing to keep track of this thing - good work, gentlemen!

Subscribing, and just wanted to provide moral support for resolution of this issue. :-) As mentioned above, resolution will allow progress on a really old issue at http://drupal.org/node/13221.

I would like to mention that the upgrade path business logic was my work and with that I am basically done with this issue. Hope I will be able to delete useless followups ("how do I apply this HEAD patch on my stable Drupal") on drupal.org :)

this is now very close. i've fixed all of the outstanding issues that i know of. could probably still use some UI love, though.

issue comments can now be sensibly edited/deleted. in the case of editing, the comment will be reassigned as the most recent comment to the issue (otherwise the metadata workflow gets wonky), and metadata changes will be applied as the most current for the issue. for deletions, if the most recent comment is deleted, then the issue metadata is rolled back to reflect the state of the next most recent comment.

in order to get the metadata table right for the first comment, it's necessary to save the original issue states somewhere. i've elected to solve this issue with a serialized array of data in the project_issues table. while not perfect, i think it's the best overall solution -- it's necessary to keep the current issue metadata in that table for efficiency in issue queue queries, and i didn't want to stick in a bunch of new columns for data that's really only set once and read for a single metadata table per issue. this solution will also enable us to display the original issue states somewhere if we wish.

i've also converted the database updates to use the batching functionality present in update.php, which should make the upgrade on d.o a bit easier...

i'll get this applied and running on scratch.drupal.org within the next couple of days so folks can test.

minor update. properly use node_save() instead of manually calling queries :)

i haven't seen any talk about rss feeds here. it would be nice if our followups could show what changed when viewed in a comment rss feed (see commentrss module). i am thinking the display there should roughly match our follow up email output. this isn't urgent but should go on the todo somewhere. i would really love to subscribe to issue rss feeds (including followups) and ditch email.

@moshe: out of scope for this patch. please file another issue for that :)

couple of enhancements for the upgrade path, from our experience w/ the scratch upgrade:

• update the comment settings for issues nodes to read/write
• change the update series to 5200, to reflect the 5.x-2.x branch
• put in some basic dependency checking for the updates. since the upgrade requires comment module and comment_upload module, it's best to just abort the updates if they aren't enabled on the site.

it's time to land this. it's had a fair amount of code review and testing, and i see no outstanding bugs. committed to 5.x-2.x -- let the angels rejoice... :)