Chongsun Ahn

Probably create a new resource, like /requisitionSettings or something like that.

Probably create a new resource, like /requisitionSettings or something like that.

Can you give an overview of the changes here? There is a lot of code to review and the objectives don't explain it well.

Can you give an overview of the changes here? There is a lot of code to review and the objectives don't explain it well.

This doesn't look right to me. This method simply creates a line item domain object based on a DTO, so why would it care about a requisition status? If you want to deal with a skipped field, you sh...

This doesn't look right to me. This method simply creates a line item domain object based on a DTO, so why would it care about a requisition status? If you want to deal with a skipped field, you should do it somewhere else.

This should be pluralized, equipment_types

This should be pluralized, equipment_types

I am also wondering where the logic is to allow something can be unskipped if status is authorized?

I am also wondering where the logic is to allow something can be unskipped if status is authorized?

Then why does the RequisitionBuilderTest seems to set skipped when status is approved?

Then why does the RequisitionBuilderTest seems to set skipped when status is approved?

Generally looks good. See my other comment.

Generally looks good. See my other comment.

This doesn't match the requirements in the JSON schema. The API requires name and code, and there is logic in the controller to enforce that, but name is not required here. The API does not require...

This doesn't match the requirements in the JSON schema. The API requires name and code, and there is logic in the controller to enforce that, but name is not required here. The API does not require isbiochemistry, iscoldchain and active, but if it is not specified the database will give an error, as I don't see where the logic gives defaults.

This doesn't seem to match the logic in the backend.

This doesn't seem to match the logic in the backend.

This is a default setting, but I believe it is set in the docker compose file.

This is a default setting, but I believe it is set in the docker compose file.

It's not clear when skipped line items can be unskipped. When AUTHORIZED and APPROVED? Or just AUTHORIZED?

It's not clear when skipped line items can be unskipped. When AUTHORIZED and APPROVED? Or just AUTHORIZED?

I don't like where this API is introduced. It isn't really part of the requisitions resource, but is rather a setting in the requisition service. We should put it somewhere else, so not to confuse ...

I don't like where this API is introduced. It isn't really part of the requisitions resource, but is rather a setting in the requisition service. We should put it somewhere else, so not to confuse clients about this resource.

Is all of this code just copied from somewhere? If so, I may not need to review in depth.

Is all of this code just copied from somewhere? If so, I may not need to review in depth.

This is quite difficult to review everything. Can you explain things, so it is easier to review?

This is quite difficult to review everything. Can you explain things, so it is easier to review?

Does this mean that the user will not be able to see this UI if they do not have permission for all the reports?

Does this mean that the user will not be able to see this UI if they do not have permission for all the reports?

No test file for this controller?

No test file for this controller?

TZUP-180 Add transforms and scripts for processing periods
TZUP-180 Add transforms and scripts for processing periods
Is it intentional to set a max value of 512MB for memory?

Is it intentional to set a max value of 512MB for memory?

It would be preferable to add jasper as a prefix here, since the other ones do as well.

It would be preferable to add jasper as a prefix here, since the other ones do as well.

I'm not sure if this will be an issue, but you may want to think about situations where this code never actually gets run because the tables don't exist when these migrations are attempted. With Fl...

I'm not sure if this will be an issue, but you may want to think about situations where this code never actually gets run because the tables don't exist when these migrations are attempted. With Flyway, these migrations would only get tried to run once and then they are marked to be never run again.