Flatten existing comments

lilon - February 19, 2009 - 23:31
Project:Flatcomments
Version:6.x-1.x-dev
Component:Code
Category:feature request
Priority:normal
Assigned:Unassigned
Status:closed
Description

Or only on comments created once it is installed?

thanks

#1

Heine - February 23, 2009 - 20:44
Status:active» fixed

Only on comments created since it is installed and configured.

#2

fabrizioprocopio - March 6, 2009 - 17:27

Do you plan to create a feature to make all the foregoing comments flat?

#3

Heine - March 6, 2009 - 17:30

No, but perhaps someone else will. See also http://drupal.org/node/338041.

#4

dragonwize - March 7, 2009 - 04:51
Title:Is flatcomment working retroactively as well?» Flatten previouse comments
Category:support request» feature request
Status:fixed» postponed

This would definitely be a nice feature but since it would only be something the you ever really use once it would probably be best as a seperate module that you can turn on, use, then turn off to keep the module from being bloated with unused code.

Marking this as postponed. Don't know when I will get around to this but it is not the first item on my list so patches are welcome.

#5

dragonwize - March 7, 2009 - 05:47
Title:Flatten previouse comments» Flatten existing comments

#6

JirkaRybka - May 16, 2009 - 13:55
Version:6.x-1.0» 6.x-1.x-dev
Status:postponed» needs review

I have implemented this, so attaching for review (although I tested it pretty carefully myself).

Per #4, it's done as a separate module 'flatcomments_flatten', which lives in a 'flatcomments_flatten' subdirectory under 'flatcomments'. It doesn't really bloat the codebase with unused code, as the .module file only just implements hook_menu() and hook_help(), all the rest is hidden in an include file (so it might be possible to merge into flatcomments.module without problems), but still this approach gives the benefit of allowing the "enable - use - disable" workflow, so I left it as module. Disclaimer: I mean it as a contribution to the flatcomments project - I have no intention of maintaining this as a separate project.

If enabled, it adds a new tab "Flatten comments" on 'admin/content/comment' page, where one can select content type(s) to process, and start the operation. The point of this form is to allow only certain content types to be processed, as the site admin sees fitting (because it's a permanent change to the database, not suitable for evaluation purposes, even if a content type was set to flat mode currently), as well as confirm the operation (there's a description with a warning, and an empty default requiring the user to make an explicite selection).

The processing is done in a batch, ensuring no problems even on large sites. Although there are just simple queries, and pretty fast due to indexes, still it took 3-4 batch iterations to finish all content types on my site.

Let me to elaborate a little on the actual processing inside flatcomments_flatten_one(): We need to build the 'thread' field there, to contain a van-code corresponding to the order of each comment [in the flat list], forming new threads in the same order. I considered several methods how to get serial number for each comment found for a given node [with sort]:
- Use @VARIABLE in MySQL, incrementing it for each row selected (recommended on MySQL manual pages). Seems to be unsupported on PgSQL.
- Use a temporary table with auto-increment/serial field, filling it with data from the ordinary SELECT query, so that the database engine fills in the serial numbers. Seems supported on both databases, but creation of the temporary table would require db-type specific code, or ugly hacks around Schema API. It's a rather complicated approach, and since I can't test on PgSQL, I avoided this.
- Since I don't see any other way to bulk-generate the van-codes on query-level, I resorted to a simple loop in php code, which is not that slow due to fast queries being used, and the Batch API takes care of timeouts anyway. The benefit of this approach is also that the van-codes are generated by the original comment module's function, ensuring the format to be correct.

Attached patch doesn't change any existing files, it only just adds a subdirectory 'flatcomments_flatten' with three new files inside. The same is attached also as a tar.gz archive, in case someone needs it quickly, and for easier review.

AttachmentSize
flatcomments_flatten.patch 6.42 KB
flatcomments_flatten.tar_.gz 2.18 KB

#7

dragonwize - May 16, 2009 - 21:40
Status:needs review» needs work

This is awesome JirkaRybka. I love the use of the batch process api.

I've committed your work so far with some changes:

  • Changed the module name to Flatcomments existing. I think that explains the module functionality a little better as Flatcomments technically "flattens" as well just acts on new comments so users may think they need this module to "flatten" which is not necessarily the case.
  • Added more access arguments to the menu item. This is a major task that can have serious consequences and many sites give out "administer comments" to roles like moderators because the perms are not broken down further. Because of that I added "administer nodes" and "administer content types" to the menu item access requirements.
  • Added a message on enable the gives a link to admin/content/comment/flatten so user can easily go to the page.

One feature I would like to be added though before this is put out in an official release is a confirmation screen after execute. As mentioned before this is a major task that can not be undone and while there is bold text on the page, I think even more user cautioning and approval would be good.

#8

JirkaRybka - May 16, 2009 - 22:20

Thanks. I didn't look at the changed code yet - it's too late night now... But the mentioned changes all look quite good from what I read here. I basically relied on the "enable - use - disable" workflow, so I didn't expect other users to meet the module enabled. But of course, more security is always a good thing.

I'm not sure, when I'll have another free time for this - maybe in a few days, but no promises... I hope I can continue on this soon.

#9

JirkaRybka - May 17, 2009 - 20:28
Status:needs work» needs review

Allright, I have a patch now. Hell, it took me a while to figure this out! A confirmation screen sounds easy, but unlike content deletions and such, where the node-ID in question is passed in the URL, we need to pass an array of (arbitrary named) content types past confirmation. Finally I managed to do this as a multi-pass form, so I was able to avoid redirections, additional menu paths and stuff, and FAPI provides the storage for us. BTW - Multi-pass forms are almost undocumented, they are nearly never mentioned anywhere outside the actual form.inc code. (That's why I commented on the flow.)

Additional things in the patch:
- Typo fix and clarification in hook_enable() text
- Simplified syntax and removal of commented-out-cruft in access check.

No changes to the actual processing.

AttachmentSize
flatcomments-existing-confirm.patch 5.92 KB

#10

JirkaRybka - May 17, 2009 - 20:44

Plus updated README.txt

AttachmentSize
flatcomments-existing-confirm2.patch 6.81 KB

#11

dragonwize - May 18, 2009 - 02:59
Status:needs review» fixed

Great work JirkaRybka. This has worked well in all my testing. Committed.

I am working on another feature for the main flatcomments module. When that is done, should be pretty soon, I will be posting a new release that will contain this.

Thanks again for all your work.

#12

System Message - June 1, 2009 - 03:00
Status:fixed» closed

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

#13

Drake - October 1, 2009 - 21:37

Hi guys

I've got some issue with advanced forum.
I used teh flat comments and since than I cannot delete any comments in advanced forum.
The delete link provides this path

../comment/delete/?token=67ecf

and this does not work because there is no ID before the ?

if I change the link manually to ../comment/delete/75?token=67ecf (75 is comment ID) then the deleting works perfectly...

 
 

Drupal is a registered trademark of Dries Buytaert.