Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

HIPP-364: Add service layer in api-hub-applications #17

Merged
merged 15 commits into from
Feb 28, 2023
Merged

HIPP-364: Add service layer in api-hub-applications #17

merged 15 commits into from
Feb 28, 2023

Conversation

PaulCDurham
Copy link
Contributor

Copy link
Contributor

@llmikeyj llmikeyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

llmikeyj
llmikeyj previously approved these changes Feb 20, 2023
Copy link
Contributor

@llmikeyj llmikeyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@tigdik tigdik marked this pull request as ready for review February 23, 2023 14:30
@laurahk
Copy link
Contributor

laurahk commented Feb 23, 2023

Quite a few unused imports. Just run sbt clean test to see:

[warn] /Users/hmrc/Documents/projects/api-hub-applications/app/uk/gov/hmrc/apihubapplications/controllers/ApplicationsController.scala:26:52: Unused import
[warn] import uk.gov.hmrc.apihubapplications.repositories.ApplicationsRepository
[warn]                                                    ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/app/uk/gov/hmrc/apihubapplications/repositories/ApplicationsRepository.scala:21:43: Unused import
[warn] import org.mongodb.scala.bson.conversions.Bson
[warn]                                           ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/app/uk/gov/hmrc/apihubapplications/repositories/ApplicationsRepository.scala:22:41: Unused import
[warn] import org.mongodb.scala.model.Updates.{addEachToSet, combine, set}
[warn]                                         ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/app/uk/gov/hmrc/apihubapplications/repositories/ApplicationsRepository.scala:22:55: Unused import
[warn] import org.mongodb.scala.model.Updates.{addEachToSet, combine, set}
[warn]                                                       ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/app/uk/gov/hmrc/apihubapplications/repositories/ApplicationsRepository.scala:22:64: Unused import
[warn] import org.mongodb.scala.model.Updates.{addEachToSet, combine, set}
[warn]                                                                ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/app/uk/gov/hmrc/apihubapplications/services/ApplicationsService.scala:26:26: Unused import
[warn] import java.time.{Clock, LocalDateTime}
[warn]                          ^
[warn] 6 warnings found
[info] compiling 4 Scala sources to /Users/hmrc/Documents/projects/api-hub-applications/target/scala-2.13/test-classes ...
[warn] /Users/hmrc/Documents/projects/api-hub-applications/test/uk/gov/hmrc/apihubapplications/controllers/ApplicationsControllerSpec.scala:40:52: Unused import
[warn] import uk.gov.hmrc.apihubapplications.repositories.ApplicationsRepository
[warn]                                                    ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/test/uk/gov/hmrc/apihubapplications/services/ApplicationsServiceSpec.scala:22:22: Unused import
[warn] import play.api.libs.json
[warn]                      ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/test/uk/gov/hmrc/apihubapplications/services/ApplicationsServiceSpec.scala:23:27: Unused import
[warn] import play.api.libs.json.Json
[warn]                           ^
[warn] /Users/hmrc/Documents/projects/api-hub-applications/test/uk/gov/hmrc/apihubapplications/services/ApplicationsServiceSpec.scala:25:74: Unused import
[warn] import uk.gov.hmrc.apihubapplications.models.application.EnvironmentName._
[warn]

@laurahk
Copy link
Contributor

laurahk commented Feb 23, 2023

A good change to make to the codebase 👍 Just need to address the comments and avoid changing/introducing new behaviour.

.addHttpHeaders(("Content-Type", "application/json"))
.put(statusUpdateJson)
.futureValue

response.status shouldBe 400
response.status shouldBe 404
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own notes: my first thought here was, "should this be a 400 or 404"? But I guess it's 404 because we can't find the scope in a PENDING state. So it's fine as is 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check A/C of HIPP-251. I think it is correct as this test is about adding scopes to invalid environment, e.g. "env-does-not-exist"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not have an invalid environment - it is trying to change the scope on prod of a scope that is already approved. Either way - it's fine, it was just a note for myself so feel free to resolve this conversation.

Copy link
Contributor

@laurahk laurahk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the Application Lenses test to write. Everything else looks great. I've checked out the code and tested it using Postman. I'm just going to add the AC about approving a scope that is already approved as an AC so we have a record of the behaviour. Nevermind, I just saw you did that, thanks!

@tigdik tigdik merged commit 8fc7e02 into main Feb 28, 2023
@tigdik tigdik deleted the hipp-364 branch February 28, 2023 14:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants