Input format schema and Api integration added for quote and select ap…

[shoaib5261]

Input format schema and Wakam api integration added for quote and select endpoints

[AminJELLADParticeep]

Many things :
1 - rename all occurences of newpricer into "wakam" for the folder and "WakamHome" for class name
2 - you must add test for wakam Service like explained in the readme
3 - put "wakam.home" before in the start of package name.
4 - encaspule all class and object (put in the start private[wakam])
5 - put "wakam.home" in the start of each package

[AminJELLADParticeep]

you should put this before the yield (so between for and yield) and create a method named for instance "parse_wakam_response" and create a test which ... test your method

[AminJELLADParticeep]

same thing here:
"you should put this before the yield (so between for and yield) and create a method named for instance "parse_wakam_response" and create a test which ... test your method"
Moreover is there any way to merge this code with code you wrote below ?

[AminJELLADParticeep]

you have to encaspulate private[wakam]
same thing for all class and object.

[AminJELLADParticeep]

I appreciate this, but it is relevant when you are sharing code or for a tutorial. All employee (must) knows this, because it is a rule in our enterprise. That's means if you will work for another enterprise this rule won't perhaps not appear or will be be prohibed.

You can delete all comment from this class. At least you seems to prove that you understood the mechanic.

[AminJELLADParticeep]

you can rename this method in "convert_boolean_to_yes_no" , I thing it is more understandable.

[AminJELLADParticeep]

Delete all useless file like NewPricerxxx, normally you had to fill it and rename the class according to the api name.

You did some method like parse_wakam_quote_response. As we said in the readme you have to test your method. This time you can use mock.

[AminJELLADParticeep]

Use TimeUtils.now() instead of OffsetDateTime.now().

[AminJELLADParticeep]

wrong way to do that.
Do result => result return nothing so your test is a success. It is like returned "void"
You can use "succeed" from scala test to say here it is a success case

reponse.fold(
fail => ...,
_ => succeed
)

[AminJELLADParticeep]

You do not need to put a comment, you can use scalatest to ignore your test.
remplace "in" to "ignore"

[AminJELLADParticeep]

you can delete this file, because you defined somewhere else wakam quote and wakam selec data

[AminJELLADParticeep]

WakamJsonParser

normally this trait regroup all json from your class except custom situation when you need particular write or read in your code. So add all format here, if possible.

[AminJELLADParticeep]

now you can rename this class to WakamQuoteConfig

[AminJELLADParticeep]

You can clearly improve this part of the code (from line 32 to line 46).
See how.
for instance you can play with .map, getOrElse , orElse etc...

[AminJELLADParticeep]

replace new_pricer by wakam_home.
And also the UUID. you can generate a new UUID online.

[Driox]

• Il ne doit pas y avoir de modification en dehors du module 03-new_pricer
• le renomage de new_pricer en wakam rend la review impossible car on ne voit pas le diff git : il faut revert ce changement
• il y a des tests inutiles : il faut les rendre utile

[AminJELLADParticeep]

They are too much warning in your code.

[AminJELLADParticeep]

Why have you a success if you received a price ? The aims of this test is to receive an error so this case must fail.

[AminJELLADParticeep]

I think you can simplify this pattern matching :

response match {
 case \/-(Offer(price,_,_,_,_)) =>  price mustBe offer_data.price
 case other => fail(s"unexpected response: $other_response") 
}

[AminJELLADParticeep]

Why have you a success if you received a price ? The aims of this test is to receive an error so this case must fail.

[AminJELLADParticeep]

You do not need to use network to test parse_new_pricer_quote_response.

First of all this method is only used by NewPricerService and it is not used somewhere else

private[newpricer] def parse_new_pricer_quote_response(response: SuccessCase): Offer

You have to put this private only for the class

private[this] def parse_new_pricer_quote_response(response: SuccessCase): Offer

If you want to use a method which is private for your test, you can use this PrivateMethod from scala play.

Your service need WSClient and Configuration. These things are not useful for us because we do not want to use network, you can mock them, initialize the service and invoke the private method. this should be something like that :

    "parse new pricer quote api response and return an offer" in {
      val ws: WSClient          = mock[WSClient]
      val config: Configuration = mock[Configuration]
      val service               = new NewPricerService(ws = ws, config = config)
      val parse_response        = PrivateMethod[Offer](Symbol("parse_new_pricer_quote_response"))

      val api_response = SuccessCase("8722b2eb-9069-4e5d-a174-889ee5c7c06c", "OK", "241.59")
      val offer: Offer = service invokePrivate parse_response(api_response)

      offer.price mustBe offer_data.price
    }

[AminJELLADParticeep]

replace this by : import newpricer.models._

[AminJELLADParticeep]

useless import

[AminJELLADParticeep]

You can regroup this and

private[newpricer] case class NewPricerSelectConfig(key: String, partnership_code: String)

at the same file, there are sharing the same aim. Rename the file intor NewPricerConfig.

[AminJELLADParticeep]

At the end, you do not use this class. You can delete it

[AminJELLADParticeep]

o like object ?
you should rename it for beneficiary.
but why Beneficiaries contains a S ? This class represent one Beneficiary

[AminJELLADParticeep]

revert also messages.en.conf and messages.fr.conf
revert all file outside of new_pricer expect build.sbt

[AminJELLADParticeep]

revert this file

[AminJELLADParticeep]

revert this file changing UUID is only on Pricer factory

[AminJELLADParticeep]

revert this file

[AminJELLADParticeep]

they are some warning due to unused import in your test file.

[AminJELLADParticeep]

revert completely this file, it does not appear in the pr

[AminJELLADParticeep]

I mean it must not appear in the pr

[AminJELLADParticeep]

revert completely this file, it does not appear in the pr

[AminJELLADParticeep]

I mean it must not appear in the pr

[AminJELLADParticeep]

useless import.

[Driox]

Changes in this file must be revert

[AminJELLADParticeep]

these changes are mine, otherwise he can't run the tests

[Driox]

You should not change the public interface you have to implement

new_pricer_quote_config -> broker_config

[Driox]

idem

[Driox]

You need to choose : either case class are final or not

but your code have to be consistent

[Driox]

Why you don't parse all the data

In the wakam's doc, we have data about Garanties

"Garanties": [{
        "CodeGarantie": "INC",
        "MontantPrimeHT": "15.93",
        "MontantTotalCommissions": "3.35",
        "MontantTotalTaxes": "4.78",
        "MontantPrimeTTC": "20.71",
        "Plafond": "25000.00",
        "Franchise": "150.00"
    }, {
...
}]

You need to parse them and add it into the response, in the OfferItem field

[Driox]

Where did you handle the error ?

• 400 Bad Request
• 401 Unauthorized
• 524 User defined status code

[Driox]

Why you put amount with taxes into the fields that should contains the amount without taxes ?

[Driox]

You should add some test that don't call webservice / network

This test should verify your parser (success and error) with static json data

[AminJELLADParticeep]

You created a folded with a list of enumerations, you them in quote and select AND in the input format

[AminJELLADParticeep]

Can you use a json.beautifier here ?
Go to a site for that

[AminJELLADParticeep]

put a comment to explain why this test is ignored

// test ignored due to bad credential ...

[AminJELLADParticeep]

put a comment to explain why this test is ignored

[AminJELLADParticeep]

NewPricerResponse is never use, this an old relic from another time.
Delete it, you are only using SuccessCase or make this trait usefull but you have to create another case class.

[AminJELLADParticeep]

i do not know if my superior will want to ask to do create a check_response_status like for quote but for select

[AminJELLADParticeep]

When you update a cass class :
1 - update input format
2 - update method which use the cass class
3 - update your test. Currently your code does not compile in test.

[AminJELLADParticeep]

replace
import newpricer.models.enumerations.{
CommercialLatitudeRequested,
ComputerOption,
Deductible,
NomadicBusinessOption,
OccupationStatus,
Stage
}

by
import newpricer.models.enumerations._

[AminJELLADParticeep]

Some of your input format can be improved.
For instance, if you see FieldType in module "domain", it is an ADT with this enumeration :

  final case object TEXT      extends FieldType
  final case object DATE      extends FieldType
  final case object NUMBER    extends FieldType
  final case object ADDRESS   extends FieldType
  final case object PHONE     extends FieldType
  final case object FILE      extends FieldType
  final case object ENUM      extends FieldType
  final case object OBJECT    extends FieldType
  final case object BOOLEAN   extends FieldType
  final case object SIGNATURE extends FieldType

I see you are already using this, but I want to put in light your recently update about type of three fields :

InputFormat(name = "surface", kind           = NUMBER, mandatory   = true),
InputFormat(name = "movable_capital", kind   = NUMBER, mandatory   = true),
InputFormat(name = "capital_valuables", kind = NUMBER, mandatory   = true),

See if you have to update other fields.

[AminJELLADParticeep]

For this field, you have to change type String to type SplitPayment.
Change also custom write.

See also if other fields have this problem of type.

[Driox]

you should change in every Enum

override def label(): String = "XXX"

into

val label = "XXX"

[Driox]

Why did you change name of input parameters ?

NewPricerQuoteRequest vs NewPricerQuote
NewPricerConfig vs NewPricerQuoteConfig
NewPricerSelectRequest vs NewPricerSubscribe
NewPricerConfig vs NewPricerSelectConfig

[shoaib5261]

The data types are changed for (NewPricerQuote and NewPricerSelectConfig) according to the data we are receiving from NewPricer in function call. And for the config attribute as we have different configuration parameters for both quote and select api end points for wakam that's why NewPricerConfig changed to NewPricerQuoteConfig and NewPricerSelectConfig accordingly

[Driox]

I don't understand your answer

We'll try with a smaller scope : please answer why

• you rename NewPricerQuoteRequest into NewPricerQuote
• you rename NewPricerSelectRequest into NewPricerSubscribe

[shoaib5261]

I have changed NewPricerQuoteRequest to NewPricerQuote because the data we are getting in the variable request is an object of class NewPricerQuote. If you check the the file NewPricer.scala you can see that json body is parsed to NewPricerQuote object and passed to quote function as parameter as:

    for {
      pricer_request <- quote_input.input_json.validate[NewPricerQuote] ?| ()
      auth           <- broker_config                                   ?| "error.broker.login.required"
      broker_config  <- auth.validate[NewPricerQuoteConfig]             ?| ()
      result         <- service.quote(pricer_request, broker_config)    ?| ()
    } yield {
      result
    }

[Driox]

It's not a logical answer

You renamed NewPricerQuoteRequest to NewPricerQuote
So obviously you have to take NewPricerQuote as input: it's a consequence and not a cause
The same goes for NewPricerSelectRequest into NewPricerSubscribe

If you have no real reason for the renaming please revert it

[Driox]

Nevermind we'll do it

[Driox]

This kind of test are misleading

What did you expect here ?

fail.message must include("Unprocessable entity") ?

OR

fail(s"unexpected response: $result") ?

[Driox]

You should write tests in this format

val input = ...
val result = method_under_test(input...)

val expected_result = ...

result mustBe expected_result

[shoaib5261]

As in this case I am testing the failure scenario from the method and We got response from method in the form of Fail \/ Response so that's why must matcher is applied to fail case and in case we get any other response then by using the fail method from playspec the test will fail with the unexpected response.