msupply-foundation / open-msupply

Open mSupply represents our most recent advancement in the Logistics Management Information System (LMIS), expanding on more than two decades of development inherited from the well-established legacy of the original mSupply.
https://msupply.foundation/open-msupply/
Other
23 stars 14 forks source link

UpdateStockOutLine.r#type remove option #5124

Open Chris-Petty opened 1 month ago

Chris-Petty commented 1 month ago

The suggestion

There is an optional struct field UpdateStockOutLine.r#type that results in always throwing an error if you don't provide a value. In which case, it's not optional! So howsabout it isn't?

TANGENT: The front end shouldn't be able to specify the type IMO! Bad frontend code (say a plugin??? We'd NEVER write bad code 😁) could send the wrong type. The backend surely could derive the type from the parent record... it already does various queries figuring how to apply/validate/backdate the request, one more indexed query of invoice surely is negligible!

Example use case

// BEFORE
inline_init(|r: &mut UpdateStockOutLine| {
  r.id = "prescription_stock_out_line1".to_string();
  r.r#type = Some(StockOutType::Prescription);
  r.number_of_packs = Some(10.0);
});

// AFTER
inline_init(|r: &mut UpdateStockOutLine| {
  r.id = "prescription_stock_out_line1".to_string();
  r.r#type =StockOutType::Prescription;
  r.number_of_packs = Some(10.0);
});

// BEFORE 
// THROWS AN ERROR AT RUNTIME BECAUSE YOU DIDN'T SPECIFY A TYPE BECAUSE THE TYPE SYSTEM SAID IT WAS OPTIONAL
inline_init(|r: &mut UpdateStockOutLine| {
  r.id = "prescription_stock_out_line1".to_string();
  r.number_of_packs = Some(10.0);
});

// AFTER
// THE COMPILER TELLS YOU OFF BECAUSE A REQUIRED FIELD (r#type) IS MISSING
inline_init(|r: &mut UpdateStockOutLine| {
  r.id = "prescription_stock_out_line1".to_string();
  r.number_of_packs = Some(10.0);
});

Why should we invest time in this?

Annoying working with an Option when it's mandatory. Slows dev down. Makes mistakes. Type system should prevent mistakes not create them.

Are there any risks associated with this change?

Breaks frontend maybe?

How much effort is required?

I already did it but reverted the commit. 034633acfc21

Agreed Solution

Make r.r#type required?

jmbrunskill commented 1 month ago

@Chris-Petty totally agree we should just make it required...

andreievg commented 4 weeks ago

low effort, good first issue, so bumping priority to should have

Chris-Petty commented 4 weeks ago

@andreievg I literally did the changes here https://github.com/msupply-foundation/open-msupply/commit/034633acfc2142bb7aa60148dadbae3811c4b20c so I think it's too easy even for a first issue 😆. I just pulled it out into an issue because it was too much of a refactor to sneak into a PR I was doing at the time

THough I suppose there is checking I didn't break the front end!

lache-melvin commented 2 weeks ago

Why not just remove the r#type field completely? Don't think you should be able to update an invoice line's type once it is created should you??

Chris-Petty commented 2 weeks ago

@lache-melvin good point I agree