ADR-1425: Additional validations for DSD #221
Conversation
|
| args = args | ||
| ) | ||
|
|
||
| private def pureAlcoholBigDecimalFormatter(fieldKey: String) = new BigDecimalFieldFormatter( |
There was a problem hiding this comment.
no need for BigDecimal in the name
| formatVolume(key, data).fold( | ||
| errors => Left(errors), | ||
| volumes => | ||
| if (volumes.totalLitresVolume < volumes.pureAlcoholVolume) { |
There was a problem hiding this comment.
This fails if both are negative. e.g. -100 litres of product, -10 litres of pure alcohol
There was a problem hiding this comment.
This one is being discussed with Tom
| private def checkValues(key: String, data: Map[String, String]): Either[Seq[FormError], DutySuspendedVolume] = | ||
| formatVolume(key, data).fold( | ||
| errors => Left(errors), | ||
| volumes => |
There was a problem hiding this comment.
Probably the simplest way to deal with this is check the following:
If litres > 0
check pure_alcohol > 0
check pure alcohol <= litres
else if litres = 0
check pure alcohol = 0
else [therefore litres < 0]
check pure alcohol < 0
check abs(pure_alcohol) <= abs(litres)
Probably best to spell it out (including the abs) to prioritise readability and correctness checking rather than the minimum number of cases.
There was a problem hiding this comment.
As per the screenshot in the ticket, this doesn't cover all the cases mentioned there
|
|
||
| "fail to bind when pure alcohol volume is higher than total litres value" in { | ||
| val data = Map( | ||
| "volumes.totalLitresVolume" -> "0", |
There was a problem hiding this comment.
should use a non-zero case here
|
|
||
| "fail to bind when total litres value is zero and pure alcohol volume is negative" in { | ||
| val data = Map( | ||
| "volumes.totalLitresVolume" -> "0", |
| @@ -255,8 +255,8 @@ class AdrReturnSubmissionServiceImpl @Inject() ( | |||
| AdrDutySuspendedProduct( | |||
| regime = AdrDutySuspendedAlcoholRegime.Beer, | |||
| suspendedQuantity = AdrAlcoholQuantity( | |||
| dutySuspendedBeer.totalBeer, | |||
| dutySuspendedBeer.pureAlcoholInBeer | |||
| dutySuspendedBeer.totalLitresVolume, | |||
There was a problem hiding this comment.
This would break any returns in progress but not submitted - has there been sign off for this?
One way around this is to have a trait for the models with getters which extract the appropriate fields using a common method.
There was a problem hiding this comment.
this has been addressed in the latest commit
| ) | ||
| } | ||
|
|
||
| "fail to bind when total litres value is negative and pure alcohol volume is a higher negative" in { |
There was a problem hiding this comment.
this should work, since abs(totalities) > abs(pure alcohol) - however the checks are wrong.
| formatVolume(key, data).fold( | ||
| errors => Left(errors), | ||
| volumes => | ||
| if (volumes.total < volumes.pureAlcohol) { |
There was a problem hiding this comment.
Won't be right if total and pureAlcohol is negative
-100 product, -10 lpa fails here.
| import pages.QuestionPage | ||
| import play.api.libs.json.JsPath | ||
|
|
||
| case object DutySuspendedBeerPage extends QuestionPage[DutySuspendedBeer] { | ||
| case object DutySuspendedBeerPage extends QuestionPage[DutySuspendedVolume] { |
There was a problem hiding this comment.
This doesn't use the original fields.
There was a problem hiding this comment.
the field names are being generated now on DutySuspendedVolume model, this also helps in using the custom formatter
|
|
||
| "fail to bind when pure alcohol volume is higher than total litres value" in { | ||
| val data = Map( | ||
| s"volumes.$totalVolumeKey" -> "0", |
There was a problem hiding this comment.
Should test with non-zero value
|
|
||
| "fail to bind when total litres value is zero and pure alcohol volume is negative" in { | ||
| val data = Map( | ||
| s"volumes.$totalVolumeKey" -> "0", |
There was a problem hiding this comment.
should test with a non-zero value.
No description provided.