Chongsun Ahn

These transforms must be modified to be more like the other transforms. As these were a PoC, we do not use their techniques to modify the JSON. You need to change the transforms before I can proper...

These transforms must be modified to be more like the other transforms. As these were a PoC, we do not use their techniques to modify the JSON. You need to change the transforms before I can properly review them.

Can you explain what was changed to the orderables transform? I cannot tell from the code change.

Can you explain what was changed to the orderables transform? I cannot tell from the code change.

I assume there are no other instances of CCE in this repo.

I assume there are no other instances of CCE in this repo.

I assume you've done a search through the repo to check there are no other instances of CCE.

I assume you've done a search through the repo to check there are no other instances of CCE.

This review looks empty.

This review looks empty.

There seems to be something wrong with the requisition_templates, requisition_template_assignments tables. The columns_maps table has 334 rows, corresponding to the number of program_rnr_columns, w...

There seems to be something wrong with the requisition_templates, requisition_template_assignments tables. The columns_maps table has 334 rows, corresponding to the number of program_rnr_columns, which is correct. But there should not be 334 requisition templates. Each template has several columns, so the number of templates should be much smaller, corresponding to the number of programs.

I believe both programid and templateid are not optional.

I believe both programid and templateid are not optional.

We'll have to let it go for now, but I suspect we may run into issues later.

We'll have to let it go for now, but I suspect we may run into issues later.

I think we are going to need to map the values as well. In eLMIS the values are U, C and R. But in v3, they are USER_INPUT, CALCULATED, REFERENCE_DATA, STOCK_CARDS, PREVIOUS_REQUISITION. I think we...

I think we are going to need to map the values as well. In eLMIS the values are U, C and R. But in v3, they are USER_INPUT, CALCULATED, REFERENCE_DATA, STOCK_CARDS, PREVIOUS_REQUISITION. I think we can ignore the stock cards and prev requisition, but we will need to change from letters to strings.

Are you sure this will work to set all column types to TEXT? The other options I usually see are NUMERIC, CURRENCY and BOOLEAN. We may not have good validation if we set all to TEXT.

Are you sure this will work to set all column types to TEXT? The other options I usually see are NUMERIC, CURRENCY and BOOLEAN. We may not have good validation if we set all to TEXT.

I think this master_rnr_columns is supposed to be something to do with sources

I think this master_rnr_columns is supposed to be something to do with sources

I think this master_rnr_columns is supposed to be something to do with sources

I think this master_rnr_columns is supposed to be something to do with sources

I don't think this requisition prefix is supposed to be here

I don't think this requisition prefix is supposed to be here

I think role_assignments should be something else

I think role_assignments should be something else

Do we need all these Java classes in apidto? Can we just not include certain properties?

Do we need all these Java classes in apidto? Can we just not include certain properties?

Why not just use a simple for loop for the line items? This seems more confusing.

Why not just use a simple for loop for the line items? This seems more confusing.

I don't think we should return null if we can't get a token. This could introduce a NullPointerException somewhere else. Maybe log a warning and return an empty list.

I don't think we should return null if we can't get a token. This could introduce a NullPointerException somewhere else. Maybe log a warning and return an empty list.

Are we sure PCMT will not change this at some point in the future? If it does, this code will need to be recompiled.

Are we sure PCMT will not change this at some point in the future? If it does, this code will need to be recompiled.

Seems like it would be a simple thing to make this configurable in application.properties.

Seems like it would be a simple thing to make this configurable in application.properties.