Fwd: Revue de code

[Jean Helou]

Cross posting du paris scala user group pour une petite code review :). Si le cross-posting pose un problème dites le moi et je ne recommencerai pas :)

merci !

---------- Forwarded message ----------
From: Jean Helou <jean....@gmail.com>
Date: 2013/1/25
Subject: Revue de code
To: paris-scala...@googlegroups.com


Bonsoir, 

J'aimerai avoir votre opinion sur un petit bout de "vrai" code. Je continue mes expériences avec play2 et j'ai fait un petit plugin pour gérer la connection avec le service OAuth2 de Google. Le code fonctionne et je l'utilise dans une appli blanche que je code quand j'ai un peu de temps mais j'aimerai le tester et là j'avoue que je sèche un peu. 

La première version de mon code ressemble à ça (version complete du fichier sur github): 

 object GoogleOAuth2 extends OAuth2(GoogleOAuth2Settings) {


    lazy val signIn = s"${settings.signInUrl}?client_id=${settings.clientId}&redirect_uri=${settings.redirectUri}"+


      "&response_type=code&approval_prompt=force&hd=xebia.fr"+


      "&scope=https://www.googleapis.com/auth/userinfo.email+https://www.googleapis.com/auth/userinfo.profile"



    def requestAccessTockenWS(code: String): Future[Response] =


      WS.url(settings.accessTokenUrl).post(


        Map("client_id" -> Seq(settings.clientId), "client_secret" -> Seq(settings.clientSecret), "code" -> Seq(code), "redirect_uri" -> Seq(settings.redirectUri), "grant_type" -> Seq("authorization_code")))


  }

Je l'ai changée de la façon suivante (version complete du fichier sur github )

  abstract class GoogleOAuth2[U <: GoogleOAuth2Settings](override val settings: U) extends OAuth2(settings) {


    lazy val signIn = s"${settings.signInUrl}?client_id=${settings.clientId}&redirect_uri=${settings.redirectUri}"+


      "&response_type=code&approval_prompt=force&hd=xebia.fr"+


      "&scope=https://www.googleapis.com/auth/userinfo.email+https://www.googleapis.com/auth/userinfo.profile"



    def url = WS.url _



    def requestAccessTockenWS(code: String): Future[Response] = {


      val u = url(settings.accessTokenUrl)


      val params = Map("client_id" -> Seq(settings.clientId), "client_secret" -> Seq(settings.clientSecret), "code" -> Seq(code), "redirect_uri" -> Seq(settings.redirectUri), "grant_type" -> Seq("authorization_code"))


      u.post(params)


    }

  }


  object GoogleOAuth2 extends GoogleOAuth2(GoogleOAuth2Settings)

Le test, lui, ressemble à ça (version complete sur github :

"GoogleOAuth2" should {


    import play.api.libs.ws.WS


    import play.api.libs.ws.Response


    object TestSettings extends GoogleOAuth2Settings {


      override lazy val clientId = testConfig("oauth2.google.cliend_id")


      override lazy val clientSecret = testConfig("oauth2.google.cliend_secret")


      override lazy val signInUrl = testConfig("oauth2.google.signInUrl")


      override lazy val accessTokenUrl = testConfig("oauth2.google.accessTokenUrl")


      override lazy val userInfoUrl = testConfig("oauth2.google.userInfoUrl")


      override lazy val redirectUri = testConfig("oauth2.google.redirect_uri")


    }


    class MockedGoogleOauth2 extends GoogleOAuth2(TestSettings) {


      override def url = testUrl _






      lazy val requestHolder: WS.WSRequestHolder = mock[WS.WSRequestHolder]






      def testUrl(s: String): WS.WSRequestHolder = {


        requestHolder.url returns s


        requestHolder


      }


    }






    "requestAccessTokens" in {


      import concurrent._


      import ExecutionContext.Implicits.global


      import concurrent.duration._


      import play.api.http.{ Writeable, ContentTypeOf }


      import org.mockito.Matchers.{ eq => mEq }


      import org.mockito.Matchers


      val underTest = new MockedGoogleOauth2()


      val mockWs = underTest.requestHolder


      val fakeResponse = mock[Response]


      val expected = Map("client_id" -> Seq("client_id"), "client_secret" -> Seq("client_secret"), "code" -> Seq("code"), "redirect_uri" -> Seq("http://redirect"), "grant_type" -> Seq("authorization_code"))


      type Params = Map[String, Seq[String]]


      mockWs.post[Params](mEq(expected))(Matchers.any[Writeable[Params]], Matchers.any[ContentTypeOf[Params]]) returns Future.successful(fakeResponse)






      //when



      val willBeResponse = underTest.requestAccessTockenWS("code")






      there was one(mockWs).post(mEq(expected))(any, any)


      Await.result(willBeResponse, 5 milli) must beEqualTo(fakeResponse)






    }



  }




Spécifiquement pour la méthode, je pense que je suis allé trop loin, mais pour ce qui suit des tests me semblent nécessaires et le mécanisme que je vais utiliser pour tester sera le même. 






De plus j'ai l'impression que la seconde version sera plus facile à mocker pour l'utilisateur de ce petit bout de code (moi probablement) et la technique sera sans doute la même : créer un val ou un def qui servira de provider et l'overrider pour les tests. 





lazy val requestUserInfoUrl = WS.url(settings.userInfoUrl)




 def requestAccessToken(code: String)(implicit executionContext: ExecutionContext): Future[String] = {


      import scala.concurrent._


      val willBeResponse = requestAccessTockenWS(code)


      val actual: Future[Option[String]] = willBeResponse.map(response => (response.json \ "access_token").asOpt[String])


      actual.flatMap(optS => optS.map(Future.successful(_)).getOrElse(Future.failed(new IllegalArgumentException("OAuth crendential error"))))


    }


    def authenticate[T](code: String)(implicit reads: Reads[T], executionContext: ExecutionContext) = requestAccessToken(code).flatMap(x => requestUserInfo(x))



    def requestUserInfo[T](accessToken: String)(implicit reads: Reads[T], executionContext: ExecutionContext): Future[T] = {


      val requestUrl = requestUserInfoUrl.withQueryString("access_token" -> accessToken)


      user(requestUrl.get().map(_.body))


    }  







Avis et suggestions sont les bienvenus :)






Merci






Jean