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.