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