Re: [AppDB] Allow maintainer request along with app submission

[Chris Morgan]

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