Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Are we translating requisition_line.average_monthly_consumption correctly? #5184

Open
Chris-Petty opened this issue Oct 22, 2024 · 4 comments · May be fixed by #6042
Open

Are we translating requisition_line.average_monthly_consumption correctly? #5184

Chris-Petty opened this issue Oct 22, 2024 · 4 comments · May be fixed by #6042
Assignees
Labels
bug Something is borken Priority: Must Have The product will not work without this Team Ruru 🦉 Andrei, Roxy, Ferg
Milestone

Comments

@Chris-Petty
Copy link
Contributor

Chris-Petty commented Oct 22, 2024

What went wrong? 😲

Month length is 30 rather than 365/12

/// Number of days in a month (used in AMC calculation)
pub const NUMBER_OF_DAYS_IN_A_MONTH: f64 = 30.0;

In OMS we're doing 30 for the length of a month.

https://github.com/msupply-foundation/mobile/blob/38602080d644736191062a02c024a1c7e804678c/src/database/utilities/constants.js#L16
In Mobile after much debate the consensus was to use 365/12 religiously.

OG mostly uses 365.25/12. IDK why mobile opted for 365 😆.

We store as average monthly consumption rather than (average )daily consumption

average_monthly_consumption: (data.daily_usage * NUMBER_OF_DAYS_IN_A_MONTH).ceil(),

On pull we translate we convert from daily to monthly and ceil() the value. My concern here is that at the translation layer we're applying a different month length constant and rounding saving it to the record - this could break the math of any of the other fields that were calculated based on the daily_consumption and a different length of month right? So possibly subtly breaking past data. This would bleed back to the central server if the records were pushed again.

Why do we take the ceil()? I'm afraid again of how this may force a facet into all calculations that clients may not be expecting (say in future plugin based suggestions).

Why didn't we just use average_daily_consumption? I'm guessing probably because OMS frontend found it was only showing AMC (for now 😉), so thought to just make the schema match?

Expected behaviour 🤔

How to Reproduce 🔨

Steps to reproduce the behaviour:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Your environment 🌱

  • Open mSupply Version:
  • Legacy mSupply Central Server Version:
  • Platform:
  • Database type:
@Chris-Petty Chris-Petty added bug Something is borken question Further information is requested needs triage needs daily triage New bug to be triaged by the release team labels Oct 22, 2024
@lache-melvin lache-melvin removed the needs daily triage New bug to be triaged by the release team label Oct 22, 2024
@andreievg andreievg added Needs refinement Priority: Must Have The product will not work without this and removed needs triage labels Oct 30, 2024
@andreievg
Copy link
Collaborator

Setting needs refinement and high priority to be captured in refinement session

@andreievg
Copy link
Collaborator

Refinement, use 365.25/12, an no ceil()

@andreievg andreievg added this to the Refactor fortnight milestone Oct 31, 2024
@andreievg
Copy link
Collaborator

Also check where NUMBER_OF_DAYS_IN_A_MONTH is used

@andreievg andreievg removed question Further information is requested Needs refinement labels Oct 31, 2024
@jmbrunskill
Copy link
Contributor

See this in front end...

  const monthlyConsumptionThisPeriod =
    adjustedQuantityConsumed / (periodLength / 30); // 30 days in a month

@roxy-dao roxy-dao self-assigned this Jan 14, 2025
@roxy-dao roxy-dao modified the milestones: Refactor fortnight, v2.6.0 Jan 14, 2025
@roxy-dao roxy-dao linked a pull request Jan 14, 2025 that will close this issue
4 tasks
@roxy-dao roxy-dao added the Team Ruru 🦉 Andrei, Roxy, Ferg label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is borken Priority: Must Have The product will not work without this Team Ruru 🦉 Andrei, Roxy, Ferg
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants