OLMIS-7793: Changed AMC calculation for SM facilities

[anawrotsoldevelo]

Jira link: https://openlmis.atlassian.net/browse/OLMIS-7793

---

This change is 

[sonarcloud]

Quality Gate failed

Failed conditions
36.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[pwargulak]

I'll continue looking into the PR

[pwargulak]

Does it has to happen here?
I see that:

• to get previosuPeriods you need requisitionToUpdate
• to get stockCardRangeSummariesToAverage you need previosuPeriods and period
• to get period you need requisitionToUpdate

you do all that, just to pass previosuPeriods and stockCardRangeSummariesToAverage to requisitionToUpdate.updateFrom() - is it becuase requisitionService?

Hmm, I think this might be a bad design on Requisition part in general.

What do you think about passing requisitionService to the updateFrom and doing all that "if isPopulateStockOnHandFromStockCards()" inside Requisition?
You would get a single call to updateFrom, always passing the requisitionService:
requisitionToUpdate.updateFrom(..., requisitionService);

[anawrotsoldevelo]

I like your proposed approach with passing requisitionService to updateFrom. I will attempt to cleanup the code accordingly.

[pwargulak]

Left some questions

[pwargulak]

I'm wondering about it.
If emergency == true then it doesn't calculate or validate. I'm thinking if there should be a log output, something like: "Calculation and Validation skipped for Requisition {name or ID} because it's emergency".

[pwargulak]

I don't know OLMIS model enough, but is it safe to change template of existing requisition?
Doesn't it mess with related RequisitionLineItems?

[pwargulak]

This is more like FYI, question, not necessary a request to change.

This looks strange, calculateAndSetStockBasedAverageConsumption checks for AVERAGE_CONSUMPTION column too. If both columns are displayed, it runs getNonSkippedFullSupplyRequisitionLineItems twice.
Inside the 1st if, the value is calculated by some helper method and set inside RequisitionLineItem, but the 2nd if uses RequisitionLineItem.calculateAndSetStockBasedAverageConsumption method to calculate (so RequisitionLineItem sets its fields by itself) - this gives impression that RequisitionLineItem is a full OOP Object with methods and simple value-object at the same time, which is bad design IMO.

I was thinking about this suggestion, but it dosn't solve all concerns:

if(template.isColumnInTemplateAndDisplayed(ADJUSTED_CONSUMPTION) || template.isColumnInTemplateAndDisplayed(AVERAGE_CONSUMPTION))  {
  for(RequisitionLineItem line : getNonSkippedFullSupplyRequisitionLineItems(orderables)) {
    if(template.isColumnInTemplateAndDisplayed(ADJUSTED_CONSUMPTION)) {
      line.setAdjustedConsumption(...);
    }

    if(template.isColumnInTemplateAndDisplayed(AVERAGE_CONSUMPTION)) {
      StockCardRangeSummaryDto summaryToAverage = findStockCardRangeSummary(
            stockCardRangeSummariesToAverage, line.getOrderable().getId());

        line.calculateAndSetStockBasedAverageConsumption(summaryToAverage,
            template, periods, previousRequisitions);
    }
  }
}

[sonarcloud]

Quality Gate failed

Failed conditions
38.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud