On Wednesday 29 November 2006 10:08 am, Alexander Nicolaysen Sørnes wrote:
Allow the user to submit a maintainer request along with an application, by ticking a checkbox. If the application is accepted, the maintainer request is accepted as well; and if it is deleted or marked as a duplicate, the request is deleted. These maintainer requests are not displayed in the queue.
Also, maintainer requests submitted by an administrator are now automatically approved.
Regards,
Alexander N. Sørnes
+ if($hResultMaint) + { + $oMaintainer = mysql_fetch_row($hResultMaint); + $oMaintainer = new Maintainer($oMaintainer[0]); + }
We don't want to overload the $oMaintainer variable here. Also, we want to use the name of the field you are accessing and not the index so we aren't asking ourselves what $oMaintainer[0] means. Also you'll want to check that mysql_fetch_row() actually returns a row prior to creating a new maintainer based upon it.
This logic isn't quite right:
+ /* Reject super maintainer request if app is deleted or a duplicate */ + if(($aClean['sSub'] == "Reject" || $aClean['sSub'] == "Delete") && $hResultMaint) + { + $oMaintainter->reject("Deleting super maintainer request along with application entry."); + } +
Rejecting an application doesn't mean deleting it so we don't want to delete or reject the maintainer request right there. Users can modify and resubmit applications after they have been rejected. The application class already handles the case where an application is deleted by calling Maintainer::deleteMaintainersForApplication() from Application::Delete().
In application.php:
+ if(makesafe($_REQUEST['bSmaintainerRequest']))
We should make the variable explicit, bSuperMaintainerRequest makes the purpose of this variable more clear. It's probably better to add a new variable to the class that we document as being used during application creation. That way we can modify Application::GetOutputEditorValues() to retrieve the value from the $aValues array passed in. This avoids having to makesafe() a $_REQUEST value, something that we really don't want to do anymore because eventually I think we'll want to clear out all non-standard entries in $_REQUEST as an additional protection against attacks from poorly checked input.
The text here in the queries is too specific:
"AND queued = '?' AND maintainReason != 'This user submitted the application; auto-queued.' ORDER by submitTime";
The query will fail if the text changes and we should assume it will. We need to do something like:
select count(*) as queued_maintainers from appMaintainers, appFamily where appFamily.queued = 'true' and appMaintainers.queued = 'true' and appMaintainers.appId = appFamily.appId and appMaintainers.iSubmitterId != appMaintainers.iUserId;
We also need to add a comment to getQueuedMaintainerCount() and to the sql to note that we are excluding queued maintainers that have applications queued as we will automatically submit them when the application is submitted.
Chris