I think it might be better to split this work into a different case, so we can discuss its need and if it's a valid solution.

For some background you can start reading from here #223187-26: Drupal 6 Compatibility.

CommentFileSizeAuthor
#14 fix_pass_by_reference.patch986 bytesOgredude
#12 casetracker.tar_.gz5.79 KBamitaibu
#11 casetracker.tar_.gz5.87 KBamitaibu
#3 casetracker.tar_.gz5.79 KBamitaibu
#1 casetracker.tar_.gz5.69 KBamitaibu

Comments

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Status: Active » Needs review
StatusFileSize
new casetracker.tar_.gz5.69 KB

Zipped module.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Status: Needs review » Needs work

Correct status.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Status: Needs work » Needs review
StatusFileSize
new casetracker.tar_.gz5.79 KB

Ok, I think this version is already worth review.

Dependencies:
1) CCK 6.x-2.1
2) Elements (http://drupal.org/project/elements)
3) Diff module (DEV version) with this patch #365140: Allow other modules to alter $node_diffs
4) Subform element (http://drupal.org/project/subform_element) - which allows putting the node edit form inside the comment form. thanks fago! :)

Follow the README.txt on setting up.

dan_aka_jack’s picture

dan_aka_jack commented

subscribing ;-)

eclipsegc’s picture

eclipsegc commented

OK, so I installed and played with this for a bit. My reactions are mixed:

It’s VERY compelling. The ability to select what cck fields participate with the casetracker revisions is REALLY awesome. It eliminates problems like "due dates" and we could use taxonomy vocab/terms to track statuses or whatever you’d like really. The comments are still comments, so the revisioning is only happening to the individual fields you enable. This would allow you to create as complex a case type as you like and pick and choose what fields are “revisable” on comment. So from a functionality perspective I really do like it.

What I don’t like is that this makes casetracker essentially dependent on 3 modules (besides cck) requires a patch to one of those (diff) and I’d personally like to see a dependency on views, so that’s 5 modules this would be dependent on. Which is totally thumbs down from me (not to mention the patch). Additionally, since we’re not providing the fields/etc, formatting this is going to be… not fun. I know it’s totally doable, but bleh. In addition to this it looks as though there’s some missing code as this only supports cases and not projects (despite the fact that it says it supports projects). Casetracker node participation has been moved to the content type edit screens (meh, but it was a little like wtf at first) and my list could go on. The diff gets shown on ever comment as well, which is big and green and red and looks like Christmas. Again I’m sure we could re-format this, but it’s very rough in its current incarnation.

So, in short, I really like the potential functionality, but I don’t like the existing implementation and it looks like there’s a LOT of work to get this to a releasable place. Too many dependencies ultimately kill it for me. Maybe we could consider this for a D7 release when we have fields in core?

Eclipse

PS: Also, the fact that elements module is looking for a new maintainer is kind of upsetting to this.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

@EclipseGc,
Thanks for the review. I'll try to answer some of the fine concerns you wrote.

dependent on 3 modules (besides cck)

Yes, but if we look on D7 we see those modules are already/ planned to be included in D7.
* CCK (fields already in core in D7).
* Elements (tableselect already in D7).
* Diff (might be in D7).
* Subform element (contrib)

requires a patch to one of those (diff)

no biggie IMO. If we decide to go for this solution, I'm sure Moshe will agree to review that patch.

there’s some missing code as this only supports cases and not projects

Nope, this is by design. If you want to define a project you create a CCK node reference. So, I'd mark this as an advantage.

which is big and green and red and looks like Christmas

I'm Jewish, so maybe Christmas is not by-design :) I agree, but again, this is just a theme function, I didn't bother about it, and just used diff's own theme. We can theme it like project*.

So, we both agree that setting up a case node takes some work, but the advantage in having exactly the fields we want (and different fields for different content types) is IMO worth it.

jmiccolis’s picture

jmiccolis commented

I've also taken a poke at this and quickly read through the code.

I'm critical of a couple things here:

  • Saving a new node revision for every comment is mild abuse of the revision system :p
  • There is currently lots of node_loading on display and that makes me worry about performance. It seems like a lot of work to do to get state changes.
  • Parts of element may go into core for D7 but for D6 we still need maintain it & the module itself isn't maintained.
  • I'm not convinced about using diff and subform_element. I'm not sure either module is designed to be used as we'd use them, and as the usage is for core functionality I don't feel too comfortable about it.
  • We'd want something to fill the role that the casetracker_basic module does now.
  • We need to sketch out a migration path.

All that being said I'd like to echo a point that EclipseGc makes - using cck/fields for state eliminates a lot of problems (and feature requests!). I'd like to explore what holding off on this until Drupal 7 means. I think we may get a lot from fields in core, I'm particularly interested in if we could attach fields to 'cases' and not the node that captures a case's initial state.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

@jmiccolis

Saving a new node revision for every comment is mild abuse of the revision system :p

It should create a new revision for every comment that it's status changed. If you enter just a comment, then casetracker isn't triggered. If it doesn't work like this, then it's a bug.

There is currently lots of node_loading on display and that makes me worry about performance.

Agree. Maybe we can use Views somehow to have a single query per node. But again, it's another dependency.

Parts of element

If it comes down to that we can drop this and build our own solution. although elements makes it so elegant :)

I'm not convinced about using diff and subform_element

I think this is a legitimate use of diff, as it is implements taxonomy, CCK, etc'. But, I'm of course open for suggestions.

We'd want something to fill the role that the casetracker_basic module does now.

If we use CCK, I think we can pragmatically create a content type.

I'm particularly interested in if we could attach fields to 'cases' and not the node that captures a case's initial state.

I didn't get this part - can you explain.

With all that said, I'm of course not trying to defend my implementation. If there's a better solution, I'd be in favor. The fact that we all agree to rely on CCK, is already a good step forward.

jmiccolis’s picture

jmiccolis commented

Even a new revision for every comment that causes a state change still qualifies as minor abuse, in my book anyhow...

Regarding attaching fields to cases - I'm hoping that the field api that's heading into core will allow us to attach fields to basically any object. Here to a 'case' as distinct from a node. This could allow us to alter the the case fields completely distinctly from altering the node that creates the case. This may or may not be possible with what goes into core, it's just how I'm hoping this could work.

I think we can agree that there are at least some legitimate concerns about using CCK here, particularly regarding all the node_loading and extra dependencies. Personally I don't think we should make this jump yet, and I'm hopeful that D7 will allow us to do this a little more cleanly.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

I have an idea instead of using revisions/ node_load - CCKasetracker will hold the information about the changed fields. it will build dummy nodes and send them to diff. Something like this (I checked it works).

function tst() {
  $old_node = new stdClass;
  $node = new stdClass; 
  $old_node->type = $node->type = 'case';
  $old_node->field_case_content_reference[0]['nid'] = 1;
  $node->field_case_content_reference[0]['nid'] = 2;

  $cols = _diff_default_cols();
  $header = array();
  $rows = _diff_body_rows($old_node, $node, array('casetracker' => TRUE, 'type' => $node->type));
  $output = theme('diff_table', $header, $rows, array('class' => 'casetracker_diff'), NULL, $cols);
}
amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
StatusFileSize
new casetracker.tar_.gz5.87 KB

And there it is. Same dependencies, but it doesn't rely on node_revisions.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
StatusFileSize
new casetracker.tar_.gz5.79 KB

Oh, and diff patch is no longer needed.

jmiccolis’s picture

jmiccolis commented

Ok, storing serializing parts of the node in the DB for every comment (while a questionable practice) certainly does avoid actually node loading. ...you definitely get points for fearlessness.

However this still leaves us dependent on an unmaintained module, and two others I’m still not convinced we should be using. As an arguments for this as a D7 implementation what you've got working isn't bad since we inherit a couple of the modules in question and the fields api may provide us with more flexibility than cck. Do any of us know what the fields-in-core landscape looks like currently? It may be worth taking a look there and seeing a) if we get anything by waiting, and b) if we could/should get involved to make use of cck/fieldapi like this easier.

Ogredude’s picture

Ogredude commented
StatusFileSize
new fix_pass_by_reference.patch986 bytes

Installing this module gives me this runtime error:

Warning: Call-time pass-by-reference has been deprecated in C:\wamp\www\drupal\sites\default
\modules\casetracker\casetracker.module on line 340

I have corrected this error (and incidentally a spelling correction in one of the comments) in this patch.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Project: Case Tracker » CCKase tracker
Version: master »
Category: task » bug

Thanks, I'll commit the fixed project later on.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Version: » 6.x-1.x-dev
Status: Needs review » Fixed

@Ogredude,
Fixed, 10x.

Now that this is a project I'll close this issue as well.

Status: Fixed » Closed (fixed)

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