Added the latest suggestion. Pushed to master. --Abhishek On Tue, 2013-04-23 at 14:53 -0500, Endi Sukma Dewata wrote:
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
-- Changed as mentioned.
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 }
-- Changed as suggested.
Everything else is good. ACK.