openlmis-fulfillment

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
I forgot about save

I forgot about save

Assume you had a json object that you submitted to /transferProperties rest API before this change. If you tried to use the same json object to create or update transferProperties now, without the ...

Assume you had a json object that you submitted to /transferProperties rest API before this change. If you tried to use the same json object to create or update transferProperties now, without the transferType attibute, it will throw an error.

You are right, I have tried to make the search API and others to have default so that those don't be breaking changes. However, the save method still requires the new attribute and if the new attribute is not supplied it will fail. This is why I thought this is a breaking change.

Thanks Mateusz Kwiatkowski I see you have updated the demo data. I have added your change to this review.

Thanks Mateusz Kwiatkowski I see you have updated the demo data. I have added your change to this review.

OLMIS-5480: Added unique constraint, updated demo data

OLMIS-5480: added transfer type property do transfer properties csv file

Did you update demo data?

Did you update demo data?

Is this a really contract breaking change? From what I see we added a new column to the resource. Also, in the endpoint, there is a default value so in case I would have an external service I would...

Is this a really contract breaking change? From what I see we added a new column to the resource. Also, in the endpoint, there is a default value so in case I would have an external service I wouldn't have to change anything. Did I miss something?

I noticed that we always looking for transfer properties by the facilityId and the templateType. Could we create a unique index for those columns? I mean to avoid situations where there be more tha...

I noticed that we always looking for transfer properties by the facilityId and the templateType. Could we create a unique index for those columns? I mean to avoid situations where there be more than one row for the given facility with the same template type.

OLMIS-5480: update transfer properties so that both order and shipment file related transfer credentials...
OLMIS-5480: update transfer properties so that both order and shipment file related transfer credentials...
OLMIS-5480: update transfer properties so that both order and shipment file related transfer credentials are persisted.

  1. … 9 more files in changeset.
No. There never was CsvColumn Builder. FileColumnBuilder was added in this commit.

No. There never was CsvColumn Builder. FileColumnBuilder was added in this commit.

Uncomment logback.xml

Do we also have CsvColumnBuilder that we need to remove?

Do we also have CsvColumnBuilder that we need to remove?

Should these be commented out?

Should these be commented out?

Thank you Nikodem Graczewski and Łukasz Lewczyński for reviewing this. I created a new review for a commit that affects the same files. The new review is for another refactoring that I did to appl...

Thank you Nikodem Graczewski and Łukasz Lewczyński for reviewing this.

I created a new review for a commit that affects the same files. The new review is for another refactoring that I did to apply the feedback I received on the sprint demo. By making it a new review, my hope is that it will be easier to review. I have marked the unresolved issues to resolved because those were resolved in a subsequent commit that went into that next review. I did not want to add that commit to this review, as that would double the effort it takes to review the same work twice.

I have resolved this issue as part of a commit that went into another review. I will mark this feedback as resolved so I can close this review on an obsolete file. https://github.com/OpenLMIS/open...

I have resolved this issue as part of a commit that went into another review. I will mark this feedback as resolved so I can close this review on an obsolete file.

https://github.com/OpenLMIS/openlmis-fulfillment/blob/b2ab87f581421c006cb0270c8a7984f659902450/src/main/java/org/openlmis/fulfillment/service/FileTemplateService.java#L40

This method is removed with a subsequent commit. I am going to resolve this issue here. https://github.com/OpenLMIS/openlmis-fulfillment/blob/b2ab87f581421c006cb0270c8a7984f659902450/src/main/java...
Point taken. I have done another round of refactoring to implement the feedback I received on the sprint demo. Instead of adding those changes to this review, I have created a new review. Hope that...

Point taken. I have done another round of refactoring to implement the feedback I received on the sprint demo. Instead of adding those changes to this review, I have created a new review. Hope that makes it easier.
The new review will have a few more additional tests so the commit could pass sonar analysis. The tests are not for new code. Those are for old code just written to satisfy sonar's 65% branch coverage quality gate. Hope it is okay to keep those in the same commit.

OLMIS-5581: Rename CsvFileTemplate to FileTemplate, increased coverage on some classes
OLMIS-5581: Rename CsvFileTemplate to FileTemplate, increased coverage on some classes
OLMIS-5581: Rename CsvFileTemplate to FileTemplate, increased coverage on some classes

    • -1
    • +1
    /performance/tests/orderFileTemplate.yml
  1. … 36 more files in changeset.
OLMIS-5036: disabled performance tests for shipments

    • -144
    • +144
    /performance/tests/shipment.yml
A good idea when renaming and changing the file content is to split them into different commits and reviews (first file renames, then separately file changes). Trying to check what was changed is p...

A good idea when renaming and changing the file content is to split them into different commits and reviews (first file renames, then separately file changes). Trying to check what was changed is painful (I assume there were some changes as the line counts changed).

Duplicates.

Duplicates.