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

[Abhishek Koneru]

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.
>