API-2532: Added logging and refactoring: (#5)

* API-2532: Added logging and refactoring:

- added `ApiLogger`
- changed type of application id from `UUID` to `String`
- removed some TODOs
- empty config sections removed

* API-2532: Addressed PR review comments.

* API-2532: Addressed second round PR review comments.

* API-2532: Addressed third round PR review comments.

* API-2532: Addressed fourth round PR review comments.

* API-2532: Addressed fifth round PR review comments.

app/uk/gov/hmrc/apisubscriptionfields/controller/ApiSubscriptionFieldsController.scala

@@ -19,6 +19,7 @@ package uk.gov.hmrc.apisubscriptionfields.controller
 import java.util.UUID
 import javax.inject.{Inject, Singleton}
 
+import play.api.Logger
 import play.api.libs.json.{JsValue, Json}
 import play.api.mvc._
 import uk.gov.hmrc.apisubscriptionfields.model.ErrorCode._
@@ -35,12 +36,14 @@ class ApiSubscriptionFieldsController @Inject() (service: SubscriptionFieldsServ
 
   private val NOT_FOUND_RESPONSE = NotFound(JsErrorResponse(SUBSCRIPTION_FIELDS_ID_NOT_FOUND, "Subscription Fields were not found"))
 
-  def getSubscriptionFields(rawAppId: UUID, rawApiContext: String, rawApiVersion: String): Action[AnyContent] = Action.async { implicit request =>
+  def getSubscriptionFields(rawAppId: String, rawApiContext: String, rawApiVersion: String): Action[AnyContent] = Action.async { implicit request =>
+    Logger.debug(s"[getSubscriptionFields] appId: $rawAppId apiContext: $rawApiContext apiVersion: $rawApiVersion")
     val eventualMaybeResponse = service.get(SubscriptionIdentifier(AppId(rawAppId), ApiContext(rawApiContext), ApiVersion(rawApiVersion)))
     asActionResult(eventualMaybeResponse)
   }
 
   def getSubscriptionFieldsByFieldsId(rawFieldsId: UUID): Action[AnyContent] = Action.async { implicit request =>
+    Logger.debug(s"[getSubscriptionFieldsByFieldsId] fieldsId: $rawFieldsId")
     val eventualMaybeResponse = service.get(SubscriptionFieldsId(rawFieldsId))
     asActionResult(eventualMaybeResponse)
   }
@@ -52,8 +55,9 @@ class ApiSubscriptionFieldsController @Inject() (service: SubscriptionFieldsServ
     } recover recovery
   }
 
-  def upsertSubscriptionFields(rawAppId: UUID, rawApiContext: String, rawApiVersion: String): Action[JsValue] = Action.async(parse.json) { implicit request =>
+  def upsertSubscriptionFields(rawAppId: String, rawApiContext: String, rawApiVersion: String): Action[JsValue] = Action.async(parse.json) { implicit request =>
       withJsonBody[SubscriptionFieldsRequest] { payload =>
+        Logger.debug(s"[upsertSubscriptionFields] appId: $rawAppId apiContext: $rawApiContext apiVersion: $rawApiVersion")
         service.upsert(SubscriptionIdentifier(AppId(rawAppId), ApiContext(rawApiContext), ApiVersion(rawApiVersion)), payload.fields) map {
           case (response, true) => Created(Json.toJson(response))
           case (response, false) => Ok(Json.toJson(response))
@@ -63,8 +67,9 @@ class ApiSubscriptionFieldsController @Inject() (service: SubscriptionFieldsServ
       } recover recovery
     }
 
-  def deleteSubscriptionFields(rawAppId: UUID, rawApiContext: String, rawApiVersion: String): Action[AnyContent] = Action.async { implicit request =>
+  def deleteSubscriptionFields(rawAppId: String, rawApiContext: String, rawApiVersion: String): Action[AnyContent] = Action.async { implicit request =>
     val identifier = SubscriptionIdentifier(AppId(rawAppId), ApiContext(rawApiContext), ApiVersion(rawApiVersion))
+    Logger.debug(s"[deleteSubscriptionFields] appId: $rawAppId apiContext: $rawApiContext apiVersion: $rawApiVersion")
 
     service.delete(identifier) map {
       case true => NoContent

app/uk/gov/hmrc/apisubscriptionfields/model/Model.scala

@@ -20,7 +20,7 @@ import java.util.UUID
 
 import scala.annotation.tailrec
 
-case class AppId(value: UUID) extends AnyVal
+case class AppId(value: String) extends AnyVal
 
 case class ApiContext(value: String) extends AnyVal
 
@@ -41,7 +41,7 @@ object SubscriptionIdentifier {
     }
 
     val parts = text.split(findSeparator(Separator))
-    Some(SubscriptionIdentifier(AppId(UUID.fromString(parts(0))), ApiContext(parts(1)), ApiVersion(parts(2))))
+    Some(SubscriptionIdentifier(AppId(parts(0)), ApiContext(parts(1)), ApiVersion(parts(2))))
   }
 }
 

app/uk/gov/hmrc/apisubscriptionfields/repository/SubscriptionFieldsIdRepository.scala

@@ -20,6 +20,7 @@ import java.util.UUID
 import javax.inject.{Inject, Singleton}
 
 import com.google.inject.ImplementedBy
+import play.api.Logger
 import play.api.libs.json._
 import play.modules.reactivemongo.MongoDbConnection
 import reactivemongo.api.DB
@@ -67,7 +68,6 @@ class SubscriptionFieldsIdMongoRepository @Inject() (mongoDbProvider: MongoDbPro
   private def createSingleFieldAscendingIndex(indexFieldKey: String, indexName: Option[String],
                                               isUnique: Boolean = false, isBackground: Boolean = true): Index = {
     Index(
-      // TODO use Hashed for index type because we index something that is a UUID like thing
       key = Seq(indexFieldKey -> Ascending),
       name = indexName,
       unique = isUnique,
@@ -76,7 +76,9 @@ class SubscriptionFieldsIdMongoRepository @Inject() (mongoDbProvider: MongoDbPro
   }
 
   override def save(subscription: SubscriptionFields): Future[Unit] = {
-    collection.find(Json.obj("id" -> subscription.id)).one[BSONDocument].flatMap {
+    val selector = Json.obj("id" -> subscription.id)
+    Logger.debug(s"[save] selector: $selector")
+    collection.find(selector).one[BSONDocument].flatMap {
       case Some(document) => collection.update(selector = BSONDocument("_id" -> document.get("_id")), update = subscription)
       case _ => collection.insert(subscription)
     }.map {
@@ -85,14 +87,20 @@ class SubscriptionFieldsIdMongoRepository @Inject() (mongoDbProvider: MongoDbPro
   }
 
   override def fetchById(id: String): Future[Option[SubscriptionFields]] = {
-    collection.find(Json.obj("id" -> id)).one[SubscriptionFields]
+    val selector = selectorById(id)
+    Logger.debug(s"[fetchById] selector: $selector")
+    collection.find(selector).one[SubscriptionFields]
   }
   override def fetchByFieldsId(fieldsId: UUID): Future[Option[SubscriptionFields]] = {
-    collection.find(Json.obj("fieldsId" -> fieldsId)).one[SubscriptionFields]
+    val selector = Json.obj("fieldsId" -> fieldsId)
+    Logger.debug(s"[fetchByFieldsId] selector: $selector")
+    collection.find(selector).one[SubscriptionFields]
   }
 
   override def delete(id: String): Future[Boolean] = {
-    collection.remove(Json.obj("id" -> id)).map {
+    val selector = selectorById(id)
+    Logger.debug(s"[delete] selector: $selector")
+    collection.remove(selector).map {
       writeResult => handleError(writeResult, s"Could not delete subscription fields for id: $id")
     }
   }
@@ -104,6 +112,9 @@ class SubscriptionFieldsIdMongoRepository @Inject() (mongoDbProvider: MongoDbPro
   }
 
   private def databaseAltered(writeResult: WriteResult): Boolean = writeResult.n > 0
+
+  private def selectorById(id: String) = Json.obj("id" -> id)
+
 }
 
 @ImplementedBy(classOf[MongoDb])

app/uk/gov/hmrc/apisubscriptionfields/service/SubscriptionFieldsService.scala

@@ -20,12 +20,14 @@ import java.util.UUID
 import javax.inject._
 
 import com.google.inject.ImplementedBy
+import play.api.Logger
 import uk.gov.hmrc.apisubscriptionfields.model._
 import uk.gov.hmrc.apisubscriptionfields.repository.{SubscriptionFields, SubscriptionFieldsIdRepository}
 
 import scala.concurrent.ExecutionContext.Implicits.global
 import scala.concurrent.Future
 
+//TODO: look at flattening this into just the class
 @ImplementedBy(classOf[RepositoryFedSubscriptionFieldsService])
 trait SubscriptionFieldsService {
   def get(identifier: SubscriptionIdentifier): Future[Option[SubscriptionFieldsResponse]]
@@ -53,6 +55,8 @@ class RepositoryFedSubscriptionFieldsService @Inject()(repository: SubscriptionF
     def create(): Future[SubscriptionFieldsResponse] =
       save(SubscriptionFields(identifier, uuidCreator.uuid(), subscriptionFields))
 
+    Logger.debug(s"[upsert] SubscriptionIdentifier: $identifier")
+
     repository.fetchById(identifier.encode()) flatMap {
       o =>
         o.fold(
@@ -64,22 +68,27 @@ class RepositoryFedSubscriptionFieldsService @Inject()(repository: SubscriptionF
   }
 
   def delete(identifier: SubscriptionIdentifier): Future[Boolean] = {
-    repository.delete(identifier.encode())
+    val id = identifier.encode()
+    Logger.debug(s"[delete] SubscriptionIdentifier: $identifier id: $id")
+    repository.delete(id)
   }
 
   def get(identifier: SubscriptionIdentifier): Future[Option[SubscriptionFieldsResponse]] = {
+    Logger.debug(s"[get] SubscriptionIdentifier: $identifier")
     for {
       fetch <- repository.fetchById(identifier.encode())
     } yield fetch.map(asResponse)
   }
 
   def get(subscriptionFieldsId: SubscriptionFieldsId): Future[Option[SubscriptionFieldsResponse]] = {
+    Logger.debug(s"[get] SubscriptionFieldsId: $subscriptionFieldsId")
     for {
       fetch <- repository.fetchByFieldsId(subscriptionFieldsId.value)
     } yield fetch.map(asResponse)
   }
 
   private def save(apiSubscription: SubscriptionFields): Future[SubscriptionFieldsResponse] = {
+    Logger.debug(s"[save] SubscriptionFields: $apiSubscription")
     repository.save(apiSubscription) map {
       _ => SubscriptionFieldsResponse(SubscriptionFieldsId(apiSubscription.fieldsId), apiSubscription.customFields)
     }

build.sbt

@@ -26,7 +26,6 @@ import uk.gov.hmrc.versioning.SbtGitVersioning
 import scala.language.postfixOps
 
 val compile = Seq(
-  // TODO update mongo to latest (6.1.0) and refactor the code
   "uk.gov.hmrc" %% "play-reactivemongo" % "5.2.0",
   ws,
   "uk.gov.hmrc" %% "microservice-bootstrap" % "6.9.0"
@@ -35,7 +34,6 @@ val compile = Seq(
 def test(scope: String = "test,it") = Seq(
   "uk.gov.hmrc" %% "hmrctest" % "2.4.0" % scope,
   "uk.gov.hmrc" %% "reactivemongo-test" % "2.0.0" % scope,
-  "org.mockito" % "mockito-core" % "2.10.0" % scope, //TODO delete
   "org.scalamock" %% "scalamock-scalatest-support" % "3.6.0" % scope,
   "org.scalatest" %% "scalatest" % "2.2.6" % scope,
   "org.scalatestplus.play" %% "scalatestplus-play" % "2.0.1" % scope,
@@ -104,8 +102,8 @@ lazy val acceptanceTestSettings =
     )
 
 lazy val scoverageSettings: Seq[Setting[_]] = Seq(
-  coverageExcludedPackages := "<empty>;Reverse.*;model.*;config.*;.*(AuthService|BuildInfo|Routes).*",
-  coverageMinimum := 83,
+  coverageExcludedPackages := "<empty>;Reverse.*;model.*;.*config.*;.*(AuthService|BuildInfo|Routes).*",
+  coverageMinimum := 79,
   coverageFailOnMinimum := true,
   coverageHighlighting := true,
   parallelExecution in Test := false

conf/app.routes

@@ -1,11 +1,9 @@
 # microservice specific routes
 
-# TODO add endpoints for field definitions CRUD operations
-
-GET         /application/:appId/context/:apiContext/version/:apiVersion        @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.getSubscriptionFields(appId: java.util.UUID, apiContext: String, apiVersion: String)
+GET         /application/:appId/context/:apiContext/version/:apiVersion        @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.getSubscriptionFields(appId: String, apiContext: String, apiVersion: String)
 
 GET         /fieldsId/:fieldsId                                                @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.getSubscriptionFieldsByFieldsId(fieldsId: java.util.UUID)
 
-PUT         /application/:appId/context/:apiContext/version/:apiVersion        @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.upsertSubscriptionFields(appId: java.util.UUID, apiContext: String, apiVersion: String)
+PUT         /application/:appId/context/:apiContext/version/:apiVersion        @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.upsertSubscriptionFields(appId: String, apiContext: String, apiVersion: String)
 
-DELETE      /application/:appId/context/:apiContext/version/:apiVersion        @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.deleteSubscriptionFields(appId: java.util.UUID, apiContext: String, apiVersion: String)
+DELETE      /application/:appId/context/:apiContext/version/:apiVersion        @uk.gov.hmrc.apisubscriptionfields.controller.ApiSubscriptionFieldsController.deleteSubscriptionFields(appId: String, apiContext: String, apiVersion: String)

conf/application-json-logger.xml

@@ -5,9 +5,10 @@
         <encoder class="uk.gov.hmrc.play.logging.JsonEncoder"/>
     </appender>
 
-    <logger name="uk.gov" level="WARN"/>
+    <logger name="uk.gov" level="${logger.uk.gov:-WARN}"/>
 
-    <root level="WARN">
+    <root level="${logger.application:-WARN}">
         <appender-ref ref="STDOUT"/>
     </root>
+
 </configuration>

conf/application.conf

@@ -22,6 +22,7 @@ appUrl="http://"${appName}".protected.mdtp"
 # ~~~~
 # Additional play modules can be added here
 play.modules.enabled += "com.kenshoo.play.metrics.PlayModule"
+play.modules.enabled += "uk.gov.hmrc.play.config.inject.ConfigModule"
 play.modules.enabled += "play.modules.reactivemongo.ReactiveMongoHmrcModule"
 
 # Global request handler
@@ -36,8 +37,6 @@ play.http.requestHandler = "play.api.http.GlobalSettingsHttpRequestHandler"
 # timeout 15 minutes after login (regardless of user activity).
 # session.maxAge=900
 
-## TODO - see docs for env specific config
-
 # Secret key
 # ~~~~~
 # The secret key is used to secure cryptographics functions.
@@ -149,15 +148,6 @@ microservice {
 wiremock-port = 11111
 wiremock-port = ${?WIREMOCK_PORT}
 
-# TODO discover if this is needed
-Stub {
-
-    microservice {
-        services {
-        }
-    }
-}
-
 Dev {
 
     microservice {
@@ -169,9 +159,6 @@ Dev {
                 enabled = false
             }
         }
-
-        services {
-        }
     }
 }
 
@@ -186,15 +173,5 @@ Prod {
                 enabled = true
             }
         }
-
-        services {
-        }
-    }
-}
-
-Test {
-    microservice {
-        services {
-        }
     }
 }

test/acceptance/ApiSubscriptionFieldsSpec.scala

@@ -51,8 +51,8 @@ class ApiSubscriptionFieldsSpec extends AcceptanceTestSpec
     .withHeaders(RequestHeaders.ACCEPT_HMRC_JSON_HEADER, RequestHeaders.CONTENT_TYPE_HEADER)
     .withJsonBody(Json.toJson(contents))
 
-  def fieldsEndpoint(appId: UUID, apiContext: String, apiVersion: String) =
-    s"/application/${appId.toString}/context/$apiContext/version/$apiVersion"
+  def fieldsEndpoint(appId: String, apiContext: String, apiVersion: String) =
+    s"/application/$appId/context/$apiContext/version/$apiVersion"
 
   feature("Api-Subscription-Fields") {
     Logger.logger.info(s"App.mode = ${app.mode.toString}")

test/unit/uk/gov/hmrc/apisubscriptionfields/controller/ApiSubscriptionFieldsControllerSpec.scala

@@ -17,7 +17,7 @@
 package unit.uk.gov.hmrc.apisubscriptionfields.controller
 
 import org.scalamock.scalatest.MockFactory
-import play.api.libs.json.{JsDefined, JsString, Json}
+import play.api.libs.json.{JsDefined, JsString}
 import play.api.mvc._
 import play.api.test.Helpers._
 import play.api.test._

test/unit/uk/gov/hmrc/apisubscriptionfields/repository/SubscriptionFieldsIdsRepositorySpec.scala

@@ -18,6 +18,7 @@ package unit.uk.gov.hmrc.apisubscriptionfields.repository
 
 import java.util.UUID
 
+import org.scalamock.scalatest.MockFactory
 import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
 import reactivemongo.api.DB
 import reactivemongo.bson.BSONDocument
@@ -35,7 +36,8 @@ class SubscriptionFieldsIdsRepositorySpec extends UnitSpec
   with MongoSpecSupport
   with MongoFormatters
   with JsonFormatters
-  with TestData { self =>
+  with TestData
+  with MockFactory { self =>
 
   private val mongoDbProvider = new MongoDbProvider {
     override val mongo: () => DB = self.mongo

test/util/TestData.scala

@@ -27,7 +27,7 @@ import scala.concurrent.Future
 
 trait TestData {
 
-  final val fakeAppId = UUID.randomUUID()
+  final val fakeAppId = UUID.randomUUID().toString
   final val fakeContext = "acontext"
   final val fakeVersion = "1.0.2"
   final val FakeSubscriptionIdentifier = SubscriptionIdentifier(AppId(fakeAppId),ApiContext(fakeContext),ApiVersion(fakeVersion))