Code Review

[Charlie2009]

Hi, I have been working on an internal project for my forum for a little while now, and have just about finished up. However, before I put all of it live I was hoping to have a few pros take a look and let me know if I borked anything up. It all works like it is supposed to work, but that doesn't mean it will work effciently on a high volume server.

The Project Goals:

Create a usergroup that will allow non-moderators to move spam into the moderation queue
Modify the Report system to show that a message has been moved to the moderation queue


What has been done:
I have completed the first task. I used group memberships and the built-in promotion routine to assign users to a new group based on time, posts and rep. If the members are present in the group I display a check box below the standard Report screen.

I modified report.php with the following block of code
//========== SPAM SQUAD ==========
$bSpamReport = false;
$bError = false;
$strAddReason = '';
if($_POST['chkSpam'] == '1')
{
require_once(DIR . '/includes/functions_databuild.php');
require_once(DIR . '/includes/functions.php');

//Get Post Info
if (!$postinfo AND !$postinfo = fetch_postinfo($postid))
{
$bError = true;
}

//Get Thread Info
if (!$threadinfo AND !$threadinfo = fetch_threadinfo($postinfo['threadid']))
{
$bError = true;
}

if(!$bError)
{
//Set a Spam Reported Flag and Modify the Message
$bSpamReport = true;
$strAddReason = '

This post has been sent to the moderation queue because it was flagged as SPAM';

//Unapprove the post and Rebuilt the Forum Information
unapprove_post($postid);
build_forum_counters($threadinfo[forumid]);
}
}
//========== SPAM SQUAD ==========

It checks for the existence of the checkbox, and then automatically appends some text to the "reason". Assuming that appropriate information can be found about the post, thread and forum; I unapprove the post (which may be the entire thread) and rebuild the forum counters.

Now for part two.

Part two required that I make one more change to report.php. I added a boolean parameter to let the reportitem class know that a spam report was made.
$reportobj->do_report($vbulletin->GPC['reason'] . $strAddReason, $postinfo, $bSpamReport);

I had to make a few modifications to the class_reportitem.php

I first change the function structure. I set a default value as to not upset any other calls that wouldn't know about the new boolean parameter.
function do_report($reason, &$iteminfo, $bSpamReport = false)

Here is where I get a little fuzzy about things.

I replaced:
$threadman->set('title', $subject);

with:
if($bSpamReport)
{
$threadman->set('title', $subject . ' SPAM REPORTED!!!!!');
$threadman->set('iconid', 4);
}
else
{
$threadman->set('title', $subject);
}

That will take care of changing the thread title and icon if the first report of the spam post was made by someone with the ability to flag it as spam. Since the majority of our members cannot do this, the first report may not be from one of those members.

Here are the changes that deal with a post in an existing report:

replaced:
$postman->set('title', $subject);

with:
//SPAM SQUAD ++++++++
if($bSpamReport)
{
//Get Datamanager for existing thread
$dataman =& datamanager_init('Thread', $this->registry, ERRTYPE_SILENT, 'threadpost');

//Set exiting data
$dataman->set_existing(&$rpthreadinfo);
$dataman->set('title', $subject . ' SPAM REPORTED!!!!!');
$dataman->set('iconid', 4);

$dataman->pre_save();
if (count($dataman->errors) == 0)
{
$dataman->save();
}

unset($dataman);

//Alter information for THIS post
$postman->set('title', $subject . ' SPAM REPORTED!!!!!');
$postman->set('iconid', 4);
}
else
{
$postman->set('title', $subject);
}
//++++++++ SPAM SQUAD

This will make the appropriate changes to the post as well as the existing thread. This is the first time I created a datamanager instead of just altering one that was prexisting.

I apologize if this isn't the proper way to go about this, but I am still learning. Any tips or suggestions (or critisims) would be appreciated. There is some front end security measures put in place that only allow members to to "flag as spam" when the author has 5 posts or less. However, I fear this could easily be defeated with anything that intercepts the http stream. All you would have to do is know the variable name and value. I suppose the user doing it would get caught soon thereafter, but still a concern. Would the standard procedure be to just do a second check of credentials on the server side?