Re: [Pki-devel] [PATCH] 53-2 Fixes for comments for patch 53
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