[Pki-devel] [PATCH] 53-2 Fixes for comments for patch 53

[Endi Sukma Dewata]

Some additional comments below.

On 4/23/2013 1:19 AM, Abhishek Koneru wrote:
>> 2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values
>> which are duplicated in several places. It might be better to use
>> 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these
>> values can be put in constant variables in CertRequestInfo:
>>
>>       public static final String REQ_SUCCESS = "success";
>>       public static final String REQ_ERROR = "error";
>>
>> This way we can be sure the operationResult will be used consistently.
>>
> -- Added the constant variables in CertRequestInfo

Actually they should use RES_ prefix instead of REQ_ to match 
IRequest.RES_SUCCESS/ERROR since it's for 'result', or don't use prefix 
at all.

Also, the new constants use uppercase values "SUCCESS/ERROR", but the 
the request status is in lower case. It would be more consistent to use 
lowercase values too for the result.

   Request ID: 5
   Type: enrollment
   Request Status: complete
   Operation Result: SUCCESS  <-- use lower case
   Certificate ID: 0x5

>> 3. In CertRequestInfoFactory the code for operationResult assignment
>> seems to be incorrect because it will assign 'fail' to completed
>> requests without error. Try cert-request-find, it will show all requests
>> for system certs as failed.
>>
> -- Corrected. Null value for result is given as a SUCCESS

The code can be simplified as follows:

     if (result == null || result.equals(IRequest.RES_SUCCESS)) {
         info.setOperationResult(CertRequestInfo.RES_SUCCESS);

     } else {
         info.setOperationResult(CertRequestInfo.RES_ERROR);
         String error = request.getExtDataInString(IRequest.ERROR);
         info.setErrorMessage(error);   <-- no need to check for null
     }

Everything else is good. ACK.

-- 
Endi S. Dewata