From 59089417e56cba50dd0009f0f74032149bc0483e Mon Sep 17 00:00:00 2001 From: Jonas Natten Date: Thu, 30 Jan 2025 12:41:58 +0100 Subject: [PATCH 1/5] Make sure to call `super.beforeAll()` in all `beforeAll`'s This makes sure failing logging behaves as expected --- .../ArticleSearchConverterServiceTest.scala | 2 +- .../search/ArticleSearchServiceTest.scala | 1 + .../search/SearchConverterServiceTest.scala | 2 +- .../PublishedConceptSearchServiceTest.scala | 43 ++++++++++--------- .../search/ArticleSearchServiceTest.scala | 35 ++++++++------- .../search/GrepCodesSearchServiceTest.scala | 13 +++--- .../service/search/TagSearchServiceTest.scala | 17 +++++--- .../service/search/TagSearchServiceTest.scala | 21 +++++---- .../articleapi/ArticleApiClientTest.scala | 1 + .../articleapi/ArticleApiClientTest.scala | 1 + .../draftapi/DraftApiClientTest.scala | 1 + .../LearningpathApiClientTest.scala | 1 + .../no/ndla/myndlaapi/e2e/ArenaTest.scala | 1 + .../search/LearningPathIndexServiceTest.scala | 1 + .../tapirtesting/TapirControllerTest.scala | 1 + 15 files changed, 82 insertions(+), 59 deletions(-) diff --git a/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchConverterServiceTest.scala b/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchConverterServiceTest.scala index b501bc558..f68bc6eea 100644 --- a/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchConverterServiceTest.scala +++ b/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchConverterServiceTest.scala @@ -49,7 +49,7 @@ class ArticleSearchConverterServiceTest extends UnitSuite with TestEnvironment { Tag(Seq("the", "words"), "und") ) - override def beforeAll(): Unit = {} + override def beforeAll(): Unit = super.beforeAll() test("That asSearchableArticle converts titles with correct language") { val article = TestData.sampleArticleWithByNcSa.copy(title = titles) diff --git a/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchServiceTest.scala b/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchServiceTest.scala index 1ce142cf7..71fc46ba5 100644 --- a/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchServiceTest.scala +++ b/article-api/src/test/scala/no/ndla/articleapi/service/search/ArticleSearchServiceTest.scala @@ -232,6 +232,7 @@ class ArticleSearchServiceTest ) override def beforeAll(): Unit = if (elasticSearchContainer.isSuccess) { + super.beforeAll() articleIndexService.createIndexAndAlias().get articleIndexService.indexDocument(article1).get diff --git a/audio-api/src/test/scala/no/ndla/audioapi/service/search/SearchConverterServiceTest.scala b/audio-api/src/test/scala/no/ndla/audioapi/service/search/SearchConverterServiceTest.scala index 480aa5de7..a44727441 100644 --- a/audio-api/src/test/scala/no/ndla/audioapi/service/search/SearchConverterServiceTest.scala +++ b/audio-api/src/test/scala/no/ndla/audioapi/service/search/SearchConverterServiceTest.scala @@ -81,7 +81,7 @@ class SearchConverterServiceTest extends UnitSuite with TestEnvironment { None ) - override def beforeAll(): Unit = {} + override def beforeAll(): Unit = {super.beforeAll()} test("That asSearchableAudioInformation converts titles with correct language") { val searchableAudio = searchConverterService.asSearchableAudioInformation(sampleAudio) diff --git a/concept-api/src/test/scala/no/ndla/conceptapi/service/search/PublishedConceptSearchServiceTest.scala b/concept-api/src/test/scala/no/ndla/conceptapi/service/search/PublishedConceptSearchServiceTest.scala index 0f4bc180a..78e13feeb 100644 --- a/concept-api/src/test/scala/no/ndla/conceptapi/service/search/PublishedConceptSearchServiceTest.scala +++ b/concept-api/src/test/scala/no/ndla/conceptapi/service/search/PublishedConceptSearchServiceTest.scala @@ -212,26 +212,29 @@ class PublishedConceptSearchServiceTest aggregatePaths = List.empty ) - override def beforeAll(): Unit = if (elasticSearchContainer.isSuccess) { - when(taxonomyApiClient.getSubjects).thenReturn(Success(TaxonomyData.empty)) - publishedConceptIndexService.createIndexWithName(props.DraftConceptSearchIndex) - - publishedConceptIndexService.indexDocument(concept1) - publishedConceptIndexService.indexDocument(concept2) - publishedConceptIndexService.indexDocument(concept3) - publishedConceptIndexService.indexDocument(concept4) - publishedConceptIndexService.indexDocument(concept5) - publishedConceptIndexService.indexDocument(concept6) - publishedConceptIndexService.indexDocument(concept7) - publishedConceptIndexService.indexDocument(concept8) - publishedConceptIndexService.indexDocument(concept9) - publishedConceptIndexService.indexDocument(concept10) - publishedConceptIndexService.indexDocument(concept11) - publishedConceptIndexService.indexDocument(concept12) - - blockUntil(() => { - publishedConceptSearchService.countDocuments == 12 - }) + override def beforeAll(): Unit = { + super.beforeAll() + if (elasticSearchContainer.isSuccess) { + when(taxonomyApiClient.getSubjects).thenReturn(Success(TaxonomyData.empty)) + publishedConceptIndexService.createIndexWithName(props.DraftConceptSearchIndex) + + publishedConceptIndexService.indexDocument(concept1) + publishedConceptIndexService.indexDocument(concept2) + publishedConceptIndexService.indexDocument(concept3) + publishedConceptIndexService.indexDocument(concept4) + publishedConceptIndexService.indexDocument(concept5) + publishedConceptIndexService.indexDocument(concept6) + publishedConceptIndexService.indexDocument(concept7) + publishedConceptIndexService.indexDocument(concept8) + publishedConceptIndexService.indexDocument(concept9) + publishedConceptIndexService.indexDocument(concept10) + publishedConceptIndexService.indexDocument(concept11) + publishedConceptIndexService.indexDocument(concept12) + + blockUntil(() => { + publishedConceptSearchService.countDocuments == 12 + }) + } } test("That getStartAtAndNumResults returns SEARCH_MAX_PAGE_SIZE for value greater than SEARCH_MAX_PAGE_SIZE") { diff --git a/draft-api/src/test/scala/no/ndla/draftapi/service/search/ArticleSearchServiceTest.scala b/draft-api/src/test/scala/no/ndla/draftapi/service/search/ArticleSearchServiceTest.scala index c0048ea0f..f9c4ee87e 100644 --- a/draft-api/src/test/scala/no/ndla/draftapi/service/search/ArticleSearchServiceTest.scala +++ b/draft-api/src/test/scala/no/ndla/draftapi/service/search/ArticleSearchServiceTest.scala @@ -208,22 +208,25 @@ class ArticleSearchServiceTest extends IntegrationSuite(EnableElasticsearchConta articleType = ArticleType.TopicArticle ) - override def beforeAll(): Unit = if (elasticSearchContainer.isSuccess) { - articleIndexService.createIndexAndAlias().get - - articleIndexService.indexDocument(article1).get - articleIndexService.indexDocument(article2).get - articleIndexService.indexDocument(article3).get - articleIndexService.indexDocument(article4).get - articleIndexService.indexDocument(article5).get - articleIndexService.indexDocument(article6).get - articleIndexService.indexDocument(article7).get - articleIndexService.indexDocument(article8).get - articleIndexService.indexDocument(article9).get - articleIndexService.indexDocument(article10).get - articleIndexService.indexDocument(article11).get - - blockUntil(() => articleSearchService.countDocuments == 11) + override def beforeAll(): Unit = { + super.beforeAll() + if (elasticSearchContainer.isSuccess) { + articleIndexService.createIndexAndAlias().get + + articleIndexService.indexDocument(article1).get + articleIndexService.indexDocument(article2).get + articleIndexService.indexDocument(article3).get + articleIndexService.indexDocument(article4).get + articleIndexService.indexDocument(article5).get + articleIndexService.indexDocument(article6).get + articleIndexService.indexDocument(article7).get + articleIndexService.indexDocument(article8).get + articleIndexService.indexDocument(article9).get + articleIndexService.indexDocument(article10).get + articleIndexService.indexDocument(article11).get + + blockUntil(() => articleSearchService.countDocuments == 11) + } } test("That getStartAtAndNumResults returns SEARCH_MAX_PAGE_SIZE for value greater than SEARCH_MAX_PAGE_SIZE") { diff --git a/draft-api/src/test/scala/no/ndla/draftapi/service/search/GrepCodesSearchServiceTest.scala b/draft-api/src/test/scala/no/ndla/draftapi/service/search/GrepCodesSearchServiceTest.scala index 69524c36c..805158bd4 100644 --- a/draft-api/src/test/scala/no/ndla/draftapi/service/search/GrepCodesSearchServiceTest.scala +++ b/draft-api/src/test/scala/no/ndla/draftapi/service/search/GrepCodesSearchServiceTest.scala @@ -42,14 +42,17 @@ class GrepCodesSearchServiceTest extends IntegrationSuite(EnableElasticsearchCon val articlesToIndex: Seq[Draft] = Seq(article1, article2, article3, article4) - override def beforeAll(): Unit = if (elasticSearchContainer.isSuccess) { - tagIndexService.createIndexWithName(props.DraftGrepCodesSearchIndex) + override def beforeAll(): Unit = { + super.beforeAll() + if (elasticSearchContainer.isSuccess) { + tagIndexService.createIndexWithName(props.DraftGrepCodesSearchIndex) - articlesToIndex.foreach(a => grepCodesIndexService.indexDocument(a)) + articlesToIndex.foreach(a => grepCodesIndexService.indexDocument(a)) - val allGrepCodesToIndex = articlesToIndex.flatMap(_.grepCodes) + val allGrepCodesToIndex = articlesToIndex.flatMap(_.grepCodes) - blockUntil(() => grepCodesSearchService.countDocuments == allGrepCodesToIndex.size) + blockUntil(() => grepCodesSearchService.countDocuments == allGrepCodesToIndex.size) + } } test("That searching for grepcodes returns sensible results") { diff --git a/draft-api/src/test/scala/no/ndla/draftapi/service/search/TagSearchServiceTest.scala b/draft-api/src/test/scala/no/ndla/draftapi/service/search/TagSearchServiceTest.scala index ea9661151..07bca1bf8 100644 --- a/draft-api/src/test/scala/no/ndla/draftapi/service/search/TagSearchServiceTest.scala +++ b/draft-api/src/test/scala/no/ndla/draftapi/service/search/TagSearchServiceTest.scala @@ -67,16 +67,19 @@ class TagSearchServiceTest extends IntegrationSuite(EnableElasticsearchContainer val articlesToIndex: Seq[Draft] = Seq(article1, article2, article3, article4) - override def beforeAll(): Unit = if (elasticSearchContainer.isSuccess) { - tagIndexService.createIndexAndAlias().get + override def beforeAll(): Unit = { + super.beforeAll() + if (elasticSearchContainer.isSuccess) { + tagIndexService.createIndexAndAlias().get - articlesToIndex.foreach(a => tagIndexService.indexDocument(a).get) + articlesToIndex.foreach(a => tagIndexService.indexDocument(a).get) - val allTagsToIndex = articlesToIndex.flatMap(_.tags) - val groupedByLanguage = allTagsToIndex.groupBy(_.language) - val tagsDistinctByLanguage = groupedByLanguage.values.flatMap(x => x.flatMap(_.tags).toSet) + val allTagsToIndex = articlesToIndex.flatMap(_.tags) + val groupedByLanguage = allTagsToIndex.groupBy(_.language) + val tagsDistinctByLanguage = groupedByLanguage.values.flatMap(x => x.flatMap(_.tags).toSet) - blockUntil(() => tagSearchService.countDocuments == tagsDistinctByLanguage.size) + blockUntil(() => tagSearchService.countDocuments == tagsDistinctByLanguage.size) + } } test("That searching for tags returns sensible results") { diff --git a/image-api/src/test/scala/no/ndla/imageapi/service/search/TagSearchServiceTest.scala b/image-api/src/test/scala/no/ndla/imageapi/service/search/TagSearchServiceTest.scala index cc3359339..9eda66fac 100644 --- a/image-api/src/test/scala/no/ndla/imageapi/service/search/TagSearchServiceTest.scala +++ b/image-api/src/test/scala/no/ndla/imageapi/service/search/TagSearchServiceTest.scala @@ -75,15 +75,18 @@ class TagSearchServiceTest val imagesToIndex: Seq[ImageMetaInformation] = Seq(image1, image2, image3, image4) - override def beforeAll(): Unit = if (elasticSearchContainer.isSuccess) { - tagIndexService.createIndexAndAlias().get - imagesToIndex.foreach(a => tagIndexService.indexDocument(a).get) - - val allTagsToIndex = imagesToIndex.flatMap(_.tags) - val groupedByLanguage = allTagsToIndex.groupBy(_.language) - val tagsDistinctByLanguage = groupedByLanguage.values.flatMap(x => x.flatMap(_.tags).toSet) - - blockUntil(() => tagSearchService.countDocuments() == tagsDistinctByLanguage.size) + override def beforeAll(): Unit = { + super.beforeAll() + if (elasticSearchContainer.isSuccess) { + tagIndexService.createIndexAndAlias().get + imagesToIndex.foreach(a => tagIndexService.indexDocument(a).get) + + val allTagsToIndex = imagesToIndex.flatMap(_.tags) + val groupedByLanguage = allTagsToIndex.groupBy(_.language) + val tagsDistinctByLanguage = groupedByLanguage.values.flatMap(x => x.flatMap(_.tags).toSet) + + blockUntil(() => tagSearchService.countDocuments() == tagsDistinctByLanguage.size) + } } test("That searching for tags returns sensible results") { diff --git a/integration-tests/src/test/scala/no/ndla/integrationtests/draftapi/articleapi/ArticleApiClientTest.scala b/integration-tests/src/test/scala/no/ndla/integrationtests/draftapi/articleapi/ArticleApiClientTest.scala index 4740350f0..2810773ae 100644 --- a/integration-tests/src/test/scala/no/ndla/integrationtests/draftapi/articleapi/ArticleApiClientTest.scala +++ b/integration-tests/src/test/scala/no/ndla/integrationtests/draftapi/articleapi/ArticleApiClientTest.scala @@ -57,6 +57,7 @@ class ArticleApiClientTest val articleApiBaseUrl: String = s"http://localhost:$articleApiPort" override def beforeAll(): Unit = { + super.beforeAll() implicit val ec: ExecutionContextExecutorService = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) articleApi = new articleapi.MainClass(articleApiProperties) diff --git a/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/articleapi/ArticleApiClientTest.scala b/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/articleapi/ArticleApiClientTest.scala index 7477896c7..62eb97935 100644 --- a/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/articleapi/ArticleApiClientTest.scala +++ b/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/articleapi/ArticleApiClientTest.scala @@ -48,6 +48,7 @@ class ArticleApiClientTest val articleApiBaseUrl: String = s"http://localhost:$articleApiPort" override def beforeAll(): Unit = { + super.beforeAll() implicit val ec: ExecutionContextExecutorService = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) articleApi = new articleapi.MainClass(articleApiProperties) diff --git a/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/draftapi/DraftApiClientTest.scala b/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/draftapi/DraftApiClientTest.scala index 54fbabd18..b477728c5 100644 --- a/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/draftapi/DraftApiClientTest.scala +++ b/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/draftapi/DraftApiClientTest.scala @@ -49,6 +49,7 @@ class DraftApiClientTest val draftApiBaseUrl: String = s"http://localhost:$draftApiPort" override def beforeAll(): Unit = { + super.beforeAll() when(myndlaApiClient.getStatsFor(any, any)).thenReturn(Success(List.empty)) implicit val ec: ExecutionContextExecutorService = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) diff --git a/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/learningpathapi/LearningpathApiClientTest.scala b/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/learningpathapi/LearningpathApiClientTest.scala index 4b9c48c1f..58da6f0a5 100644 --- a/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/learningpathapi/LearningpathApiClientTest.scala +++ b/integration-tests/src/test/scala/no/ndla/integrationtests/searchapi/learningpathapi/LearningpathApiClientTest.scala @@ -50,6 +50,7 @@ class LearningpathApiClientTest val learningpathApiBaseUrl: String = s"http://localhost:$learningpathApiPort" override def beforeAll(): Unit = { + super.beforeAll() when(myndlaApiClient.getStatsFor(any, any)).thenReturn(Success(List.empty)) implicit val ec: ExecutionContextExecutorService = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) diff --git a/myndla-api/src/test/scala/no/ndla/myndlaapi/e2e/ArenaTest.scala b/myndla-api/src/test/scala/no/ndla/myndlaapi/e2e/ArenaTest.scala index c648e971f..a8aa105ca 100644 --- a/myndla-api/src/test/scala/no/ndla/myndlaapi/e2e/ArenaTest.scala +++ b/myndla-api/src/test/scala/no/ndla/myndlaapi/e2e/ArenaTest.scala @@ -87,6 +87,7 @@ class ArenaTest val myndlaApiArenaUrl: String = s"$myndlaApiBaseUrl/myndla-api/v1/arena" override def beforeAll(): Unit = { + super.beforeAll() implicit val ec = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) Future { myndlaApi.run() }: Unit Thread.sleep(1000) diff --git a/search-api/src/test/scala/no/ndla/searchapi/service/search/LearningPathIndexServiceTest.scala b/search-api/src/test/scala/no/ndla/searchapi/service/search/LearningPathIndexServiceTest.scala index 336dfaf51..5c9ec4c63 100644 --- a/search-api/src/test/scala/no/ndla/searchapi/service/search/LearningPathIndexServiceTest.scala +++ b/search-api/src/test/scala/no/ndla/searchapi/service/search/LearningPathIndexServiceTest.scala @@ -44,6 +44,7 @@ class LearningPathIndexServiceTest } override def beforeAll(): Unit = { + super.beforeAll() when(myndlaApiClient.getStatsFor(any, any)).thenReturn(Success(List.empty)) } diff --git a/tapirtesting/src/main/scala/no/ndla/tapirtesting/TapirControllerTest.scala b/tapirtesting/src/main/scala/no/ndla/tapirtesting/TapirControllerTest.scala index 44f065451..4d483177a 100644 --- a/tapirtesting/src/main/scala/no/ndla/tapirtesting/TapirControllerTest.scala +++ b/tapirtesting/src/main/scala/no/ndla/tapirtesting/TapirControllerTest.scala @@ -31,6 +31,7 @@ trait TapirControllerTest var server: HttpServer = _ override def beforeAll(): Unit = { + super.beforeAll() server = Routes.startJdkServerAsync(s"TapirControllerTest:${this.getClass.getName}", serverPort) {} Thread.sleep(1000) } From dfba18181764e933b15faa818d3fa200f3ef3c15 Mon Sep 17 00:00:00 2001 From: Jonas Natten Date: Thu, 30 Jan 2025 15:42:43 +0100 Subject: [PATCH 2/5] learningpath-api: Rework `seqNo` updating logic Recalculate all steps `seqNo` every update. Since we already fetch all steps no matter what this is not more expensive, but more robust. --- .../learningpathapi/ComponentRegistry.scala | 6 +- .../LearningpathApiProperties.scala | 14 -- .../LearningPathRepositoryComponent.scala | 6 + .../learningpathapi/service/ReadService.scala | 4 +- .../service/UpdateService.scala | 98 ++++----- .../LearningPathAndStepCreationTests.scala | 207 ++++++++++++++++++ 6 files changed, 264 insertions(+), 71 deletions(-) create mode 100644 learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala index ad2f46ec6..5d5dbd928 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala @@ -40,7 +40,7 @@ import no.ndla.learningpathapi.validation.{ UrlValidator } import no.ndla.network.NdlaClient -import no.ndla.network.clients.{FeideApiClient, MyNDLAApiClient, RedisClient} +import no.ndla.network.clients.{MyNDLAApiClient, RedisClient} import no.ndla.network.tapir.TapirApplication import no.ndla.search.{BaseIndexService, Elastic4sClient} @@ -60,7 +60,6 @@ class ComponentRegistry(properties: LearningpathApiProperties) with TaxonomyApiClient with NdlaClient with ConverterService - with FeideApiClient with OembedProxyClient with Elastic4sClient with DataSource @@ -76,7 +75,6 @@ class ComponentRegistry(properties: LearningpathApiProperties) with TextValidator with UrlValidator with ErrorHandling - with RedisClient with SwaggerDocControllerConfig { override val props: LearningpathApiProperties = properties override val migrator: DBMigrator = DBMigrator( @@ -100,7 +98,6 @@ class ComponentRegistry(properties: LearningpathApiProperties) lazy val clock = new SystemClock lazy val taxonomyApiClient = new TaxonomyApiClient lazy val ndlaClient = new NdlaClient - lazy val feideApiClient = new FeideApiClient lazy val languageValidator = new LanguageValidator lazy val titleValidator = new TitleValidator lazy val learningPathValidator = new LearningPathValidator @@ -108,7 +105,6 @@ class ComponentRegistry(properties: LearningpathApiProperties) var e4sClient: NdlaE4sClient = Elastic4sClientFactory.getClient(props.SearchServer) lazy val searchApiClient = new SearchApiClient lazy val oembedProxyClient = new OembedProxyClient - lazy val redisClient = new RedisClient(props.RedisHost, props.RedisPort) lazy val myndlaApiClient = new MyNDLAApiClient lazy val learningpathControllerV2 = new LearningpathControllerV2 diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/LearningpathApiProperties.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/LearningpathApiProperties.scala index f7419aee5..1afcc01f3 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/LearningpathApiProperties.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/LearningpathApiProperties.scala @@ -20,14 +20,11 @@ trait Props extends HasBaseProps with HasDatabaseProps { } class LearningpathApiProperties extends BaseProps with DatabaseProps with StrictLogging { - def IsKubernetes: Boolean = propOrNone("NDLA_IS_KUBERNETES").isDefined - def ApplicationName = "learningpath-api" def Auth0LoginEndpoint: String = s"https://${AuthUser.getAuth0HostForEnv(Environment)}/authorize" def ApplicationPort: Int = propOrElse("APPLICATION_PORT", "80").toInt def DefaultLanguage: String = propOrElse("DEFAULT_LANGUAGE", "nb") - def MaxFolderDepth: Long = propOrElse("MAX_FOLDER_DEPTH", "5").toLong def Domain: String = propOrElse("BACKEND_API_DOMAIN", Domains.get(Environment)) @@ -37,12 +34,6 @@ class LearningpathApiProperties extends BaseProps with DatabaseProps with Strict def MaxPageSize = 10000 def IndexBulkSize = 1000 - def InternalImageApiUrl: String = s"$ImageApiHost/image-api/v3/images" - def InternalImageApiRawUrl: String = s"$ImageApiHost/image-api/raw" - - def RedisHost: String = propOrElse("REDIS_HOST", "redis") - def RedisPort: Int = propOrElse("REDIS_PORT", "6379").toInt - object ExternalApiUrls { def ImageApiUrl = s"$Domain/image-api/v3/images" def ImageApiRawUrl = s"$Domain/image-api/raw" @@ -83,8 +74,6 @@ class LearningpathApiProperties extends BaseProps with DatabaseProps with Strict EnvironmentUrls("test") ++ EnvironmentUrls("staging") - def UsernameHeader = "X-Consumer-Username" - def ElasticSearchIndexMaxResultWindow = 10000 def ElasticSearchScrollKeepAlive = "1m" def InitialScrollContextKeywords: List[String] = List("0", "initial", "start", "first") @@ -117,9 +106,6 @@ class LearningpathApiProperties extends BaseProps with DatabaseProps with Strict def SearchServer: String = propOrElse("SEARCH_SERVER", "http://search-learningpath-api.ndla-local") - def RunWithSignedSearchRequests: Boolean = - propOrElse("RUN_WITH_SIGNED_SEARCH_REQUESTS", "true").toBoolean - override def MetaMigrationLocation: String = "no/ndla/learningpathapi/db/migration" override def MetaMigrationTable: Option[String] = Some("schema_version") } diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/repository/LearningPathRepositoryComponent.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/repository/LearningPathRepositoryComponent.scala index b9fb064fc..54aa01bcb 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/repository/LearningPathRepositoryComponent.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/repository/LearningPathRepositoryComponent.scala @@ -250,6 +250,12 @@ trait LearningPathRepositoryComponent extends StrictLogging { sql"delete from learningsteps where id = $learningStepId".update() } + def deleteAllPathsAndSteps(implicit session: DBSession): Try[Unit] = + for { + _ <- Try(sql"delete from learningsteps".update()) + _ <- Try(sql"delete from learningpaths".update()) + } yield () + def learningPathsWithIdBetween(min: Long, max: Long)(implicit session: DBSession = ReadOnlyAutoSession ): List[LearningPath] = { diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/ReadService.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/ReadService.scala index 12b742ed2..bf9e9c686 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/ReadService.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/ReadService.scala @@ -19,14 +19,14 @@ import no.ndla.learningpathapi.model.domain.ImplicitLearningPath.ImplicitLearnin import no.ndla.learningpathapi.model.domain.UserInfo.LearningpathCombinedUser import no.ndla.learningpathapi.model.domain.InvalidLpStatusException import no.ndla.learningpathapi.repository.LearningPathRepositoryComponent -import no.ndla.network.clients.{FeideApiClient, MyNDLAApiClient, RedisClient} +import no.ndla.network.clients.MyNDLAApiClient import no.ndla.network.model.{CombinedUser, CombinedUserRequired} import scala.math.max import scala.util.{Failure, Success, Try} trait ReadService { - this: LearningPathRepositoryComponent & FeideApiClient & ConverterService & Clock & RedisClient & MyNDLAApiClient => + this: LearningPathRepositoryComponent & ConverterService & Clock & MyNDLAApiClient => val readService: ReadService class ReadService { diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala index 800145e99..982ca9526 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala @@ -12,7 +12,8 @@ import no.ndla.common.Clock import no.ndla.common.errors.{AccessDeniedException, NotFoundException} import no.ndla.common.implicits.* import no.ndla.common.model.domain.learningpath -import no.ndla.common.model.domain.learningpath.{LearningPath, LearningPathStatus, Message, StepStatus} +import no.ndla.common.model.domain.learningpath.StepStatus.DELETED +import no.ndla.common.model.domain.learningpath.{LearningPath, LearningPathStatus, LearningStep, Message, StepStatus} import no.ndla.learningpathapi.Props import no.ndla.learningpathapi.integration.{SearchApiClient, TaxonomyApiClient} import no.ndla.learningpathapi.model.api.* @@ -22,7 +23,6 @@ import no.ndla.learningpathapi.model.domain.UserInfo.LearningpathCombinedUser import no.ndla.learningpathapi.repository.LearningPathRepositoryComponent import no.ndla.learningpathapi.service.search.SearchIndexService import no.ndla.learningpathapi.validation.{LearningPathValidator, LearningStepValidator} -import no.ndla.network.clients.{FeideApiClient, RedisClient} import no.ndla.network.model.{CombinedUser, CombinedUserRequired} import no.ndla.network.tapir.auth.TokenUser @@ -31,8 +31,7 @@ import scala.util.{Failure, Success, Try} trait UpdateService { this: LearningPathRepositoryComponent & ReadService & ConverterService & SearchIndexService & Clock & - LearningStepValidator & LearningPathValidator & TaxonomyApiClient & FeideApiClient & SearchApiClient & Props & - RedisClient => + LearningStepValidator & LearningPathValidator & TaxonomyApiClient & SearchApiClient & Props => val updateService: UpdateService class UpdateService { @@ -300,6 +299,34 @@ trait UpdateService { } } + def updateWithStepSeqNo( + learningStepId: Long, + newStatus: StepStatus, + learningPath: LearningPath, + stepToUpdate: LearningStep, + stepsToChange: Seq[LearningStep], + owner: CombinedUserRequired + ): (LearningPath, LearningStep) = inTransaction { implicit session => + val (_, updatedStep, newLearningSteps) = + stepsToChange.sortBy(_.seqNo).foldLeft((0, stepToUpdate, Seq.empty[LearningStep])) { + case ((seqNo, foundStep, steps), curr) => + val isChangedStep = curr.id.contains(learningStepId) + val (mainStep, stepToAdd) = + if (isChangedStep) (curr, curr.copy(status = newStatus)) + else (foundStep, curr) + val updatedMainStep = mainStep.copy(seqNo = seqNo) + val updatedSteps = steps :+ stepToAdd.copy(seqNo = seqNo) + val nextSeqNo = if (stepToAdd.status == DELETED) seqNo else seqNo + 1 + + (nextSeqNo, updatedMainStep, updatedSteps) + } + + val updated = newLearningSteps.map(learningPathRepository.updateLearningStep) + val lp = converterService.insertLearningSteps(learningPath, updated, owner) + val updatedPath = learningPathRepository.update(lp) + (updatedPath, updatedStep) + } + def updateLearningStepStatusV2( learningPathId: Long, learningStepId: Long, @@ -310,56 +337,27 @@ trait UpdateService { withId(learningPathId).flatMap(_.canEditLearningpath(owner)) match { case Failure(ex) => Failure(ex) case Success(learningPath) => - learningPathRepository.learningStepWithId(learningPathId, learningStepId) match { + val stepsToChange = learningPathRepository.learningStepsFor(learningPathId) + val stepToUpdate = stepsToChange.find(_.id.contains(learningStepId)) match { + case Some(ls) => ls case None => - Failure( - NotFoundException( - s"Learningstep with id $learningStepId for learningpath with id $learningPathId not found" - ) - ) - case Some(learningStep) => - val stepToUpdate = learningStep.copy(status = newStatus) - val stepsToChangeSeqNoOn = learningPathRepository - .learningStepsFor(learningPathId) - .filter(step => step.seqNo >= stepToUpdate.seqNo && step.id != stepToUpdate.id) - - val stepsWithChangedSeqNo = stepToUpdate.status match { - case StepStatus.DELETED => - stepsToChangeSeqNoOn.map(step => step.copy(seqNo = step.seqNo - 1)) - case StepStatus.ACTIVE => - stepsToChangeSeqNoOn.map(step => step.copy(seqNo = step.seqNo + 1)) - } - - val (updatedPath, updatedStep) = inTransaction { implicit session => - val updatedStep = learningPathRepository.updateLearningStep(stepToUpdate) - - val newLearningSteps = learningPath.learningsteps - .getOrElse(Seq.empty) - .filterNot(step => - stepsWithChangedSeqNo - .map(_.id) - .contains(step.id) - ) ++ stepsWithChangedSeqNo - - val lp = converterService.insertLearningSteps(learningPath, newLearningSteps, owner) - val updatedPath = learningPathRepository.update(lp) - - stepsWithChangedSeqNo.foreach(learningPathRepository.updateLearningStep) + val msg = s"Learningstep with id $learningStepId for learningpath with id $learningPathId not found" + return Failure(NotFoundException(msg)) + } - (updatedPath, updatedStep) - } + val (updatedPath, updatedStep) = + updateWithStepSeqNo(learningStepId, newStatus, learningPath, stepToUpdate, stepsToChange, owner) - updateSearchAndTaxonomy(updatedPath, owner.tokenUser).flatMap(_ => - converterService.asApiLearningStepV2( - updatedStep, - updatedPath, - props.DefaultLanguage, - fallback = true, - owner - ) - ) + updateSearchAndTaxonomy(updatedPath, owner.tokenUser).flatMap(_ => + converterService.asApiLearningStepV2( + updatedStep, + updatedPath, + props.DefaultLanguage, + fallback = true, + owner + ) + ) - } } } diff --git a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala new file mode 100644 index 000000000..e9aca7342 --- /dev/null +++ b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala @@ -0,0 +1,207 @@ +/* + * Part of NDLA backend.learningpath-api.test + * Copyright (C) 2025 NDLA + * + * See LICENSE + * + */ + +package no.ndla.learningpathapi.e2e + +import io.circe.generic.auto.* +import io.circe.syntax.EncoderOps +import no.ndla.common.CirceUtil +import no.ndla.common.model.NDLADate +import no.ndla.common.model.domain.learningpath.{EmbedType, LearningPath, StepType} +import no.ndla.common.model.domain.myndla.{ArenaGroup, MyNDLAUser, UserRole} +import no.ndla.learningpathapi.model.api.{ + EmbedUrlV2DTO, + LearningPathV2DTO, + LearningStepV2DTO, + NewLearningPathV2DTO, + NewLearningStepV2DTO +} +import no.ndla.learningpathapi.{ComponentRegistry, LearningpathApiProperties, MainClass, UnitSuite} +import no.ndla.scalatestsuite.IntegrationSuite +import org.mockito.ArgumentMatchers.{any, eq as eqTo} +import org.mockito.Mockito.{reset, spy, when, withSettings} +import org.mockito.invocation.InvocationOnMock +import org.mockito.quality.Strictness +import org.testcontainers.containers.PostgreSQLContainer +import sttp.client3.Response +import sttp.client3.quick.* + +import java.util.concurrent.Executors +import scala.concurrent.{ExecutionContext, Future} +import scala.concurrent.duration.DurationInt +import scala.util.Success + +class LearningPathAndStepCreationTests + extends IntegrationSuite( + EnableElasticsearchContainer = true, + EnablePostgresContainer = true, + EnableRedisContainer = true + ) + with UnitSuite { + + val learningpathApiPort: Int = findFreePort + val pgc: PostgreSQLContainer[_] = postgresContainer.get + val learningpathApiProperties: LearningpathApiProperties = new LearningpathApiProperties { + override def ApplicationPort: Int = learningpathApiPort + override def MetaServer: String = pgc.getHost + override def MetaResource: String = pgc.getDatabaseName + override def MetaUserName: String = pgc.getUsername + override def MetaPassword: String = pgc.getPassword + override def MetaPort: Int = pgc.getMappedPort(5432) + override def MetaSchema: String = "testschema" + override def disableWarmup: Boolean = true + } + + val someDate: NDLADate = NDLADate.of(2017, 1, 1, 1, 59) + + val learningpathApi: MainClass = new MainClass(learningpathApiProperties) { + override val componentRegistry: ComponentRegistry = new ComponentRegistry(learningpathApiProperties) { + override lazy val clock: SystemClock = mock[SystemClock](withSettings.strictness(Strictness.LENIENT)) +// override lazy val folderRepository: FolderRepository = spy(new FolderRepository) +// override lazy val userRepository: UserRepository = spy(new UserRepository) +// override lazy val userService: UserService = spy(new UserService) + override lazy val myndlaApiClient: MyNDLAApiClient = spy(new MyNDLAApiClient) + override lazy val taxonomyApiClient: TaxonomyApiClient = mock[TaxonomyApiClient] + + when(clock.now()).thenReturn(someDate) + when(myndlaApiClient.isWriteRestricted).thenReturn(Success(false)) + when(taxonomyApiClient.updateTaxonomyForLearningPath(any, any, any)).thenAnswer { (i: InvocationOnMock) => + Success(i.getArgument[LearningPath](0)) + } + } + } + + val testClock: learningpathApi.componentRegistry.SystemClock = learningpathApi.componentRegistry.clock + + val learningpathApiBaseUrl: String = s"http://localhost:$learningpathApiPort" + val learningpathApiLPUrl: String = s"$learningpathApiBaseUrl/learningpath-api/v2/learningpaths" + + override def beforeAll(): Unit = { + super.beforeAll() + implicit val ec = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) + Future { learningpathApi.run() }: Unit + Thread.sleep(1000) + } + + override def beforeEach(): Unit = { + super.beforeEach() +// reset(learningpathApi.componentRegistry.userRepository) + + learningpathApi.componentRegistry.inTransaction(implicit session => { + learningpathApi.componentRegistry.learningPathRepository.deleteAllPathsAndSteps(session) + }) + } + + override def afterAll(): Unit = { + super.afterAll() + } + + val fakeToken = + "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiIsImtpZCI6Ik9FSTFNVVU0T0RrNU56TTVNekkyTXpaRE9EazFOMFl3UXpkRE1EUXlPRFZDUXpRM1FUSTBNQSJ9.eyJodHRwczovL25kbGEubm8vbmRsYV9pZCI6Inh4eHl5eSIsImlzcyI6Imh0dHBzOi8vbmRsYS5ldS5hdXRoMC5jb20vIiwic3ViIjoieHh4eXl5QGNsaWVudHMiLCJhdWQiOiJuZGxhX3N5c3RlbSIsImlhdCI6MTUxMDMwNTc3MywiZXhwIjoxNTEwMzkyMTczLCJwZXJtaXNzaW9ucyI6WyJhcnRpY2xlczpwdWJsaXNoIiwiZHJhZnRzOndyaXRlIiwiZHJhZnRzOnNldF90b19wdWJsaXNoIiwiYXJ0aWNsZXM6d3JpdGUiLCJsZWFybmluZ3BhdGg6d3JpdGUiLCJsZWFybmluZ3BhdGg6cHVibGlzaCIsImxlYXJuaW5ncGF0aDphZG1pbiJdLCJndHkiOiJjbGllbnQtY3JlZGVudGlhbHMifQ.XnP0ywYk-A0j9bGZJBCDNA5fZ4OuGRLkXFBBr3IYD50" + + def createLearningpath(title: String, shouldSucceed: Boolean = true): LearningPathV2DTO = { + val dto = NewLearningPathV2DTO( + title = title, + description = None, + coverPhotoMetaUrl = None, + duration = None, + tags = None, + language = "nb", + copyright = None + ) + + val x = CirceUtil.toJsonString(dto) + + val res = simpleHttpClient.send( + quickRequest + .post(uri"$learningpathApiLPUrl") + .body(x) + .header("Content-type", "application/json") + .header("Authorization", s"Bearer $fakeToken") + .readTimeout(10.seconds) + ) + if (shouldSucceed) { res.code.code should be(201) } + CirceUtil.unsafeParseAs[LearningPathV2DTO](res.body) + } + + def createLearningStep(pathId: Long, title: String, shouldSucceed: Boolean = true): LearningStepV2DTO = { + val dto = NewLearningStepV2DTO( + title = title, + introduction = None, + description = None, + language = "nb", + embedUrl = Some( + EmbedUrlV2DTO( + url = "https://www.example.com/", + embedType = EmbedType.External.entryName + ) + ), + showTitle = false, + `type` = StepType.TEXT.toString, + license = None + ) + val x = CirceUtil.toJsonString(dto) + val res = simpleHttpClient.send( + quickRequest + .post(uri"$learningpathApiLPUrl/$pathId/learningsteps") + .body(x) + .header("Content-type", "application/json") + .header("Authorization", s"Bearer $fakeToken") + .readTimeout(10.seconds) + ) + if (shouldSucceed) { res.code.code should be(201) } + CirceUtil.unsafeParseAs[LearningStepV2DTO](res.body) + } + + def getLearningPath(pathId: Long, shouldSucceed: Boolean = true): LearningPathV2DTO = { + val res = simpleHttpClient.send( + quickRequest + .get(uri"$learningpathApiLPUrl/$pathId") + .header("Content-type", "application/json") + .header("Authorization", s"Bearer $fakeToken") + .readTimeout(10.seconds) + ) + if (shouldSucceed) { res.code.code should be(200) } + CirceUtil.unsafeParseAs[LearningPathV2DTO](res.body) + } + + def deleteStep(pathId: Long, stepId: Long, maybeExpectedCode: Option[Int] = Some(204)): Unit = { + val res = simpleHttpClient.send( + quickRequest + .delete(uri"$learningpathApiLPUrl/$pathId/learningsteps/$stepId") + .header("Content-type", "application/json") + .header("Authorization", s"Bearer $fakeToken") + .readTimeout(10.seconds) + ) + maybeExpectedCode match { + case None => + case Some(expectedCode) => res.code.code should be(expectedCode) + } + } + + test("That sequence numbers of learningsteps are updated correctly") { + val x = createLearningpath("Test1") + val s1 = createLearningStep(x.id, "Step1") + val s2 = createLearningStep(x.id, "Step2") + val s3 = createLearningStep(x.id, "Step3") + val s4 = createLearningStep(x.id, "Step4") + val s5 = createLearningStep(x.id, "Step5") + val pathBeforeDelete = getLearningPath(x.id) + pathBeforeDelete.learningsteps.map(_.seqNo) should be(Seq(0, 1, 2, 3, 4)) + + deleteStep(x.id, s1.id) + deleteStep(x.id, s1.id, None) + deleteStep(x.id, s1.id, None) + deleteStep(x.id, s1.id, None) + deleteStep(x.id, s1.id, None) + + val path = getLearningPath(x.id) + path.learningsteps.map(_.seqNo) should be(Seq(0, 1, 2, 3)) + } + +} From 441deddf7671eb866a70e9de1a41bb576c71a902 Mon Sep 17 00:00:00 2001 From: Jonas Natten Date: Mon, 3 Feb 2025 06:51:11 +0100 Subject: [PATCH 3/5] learningpath-api: Stop allowing deleting steps multiple times --- .../learningpathapi/ComponentRegistry.scala | 2 +- .../service/UpdateService.scala | 5 +++- .../LearningPathAndStepCreationTests.scala | 28 ++++++++----------- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala index 5d5dbd928..f2324e24e 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/ComponentRegistry.scala @@ -40,7 +40,7 @@ import no.ndla.learningpathapi.validation.{ UrlValidator } import no.ndla.network.NdlaClient -import no.ndla.network.clients.{MyNDLAApiClient, RedisClient} +import no.ndla.network.clients.MyNDLAApiClient import no.ndla.network.tapir.TapirApplication import no.ndla.search.{BaseIndexService, Elastic4sClient} diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala index 982ca9526..b0c0aaaba 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala @@ -339,10 +339,13 @@ trait UpdateService { case Success(learningPath) => val stepsToChange = learningPathRepository.learningStepsFor(learningPathId) val stepToUpdate = stepsToChange.find(_.id.contains(learningStepId)) match { - case Some(ls) => ls + case Some(ls) if ls.status == DELETED && newStatus == DELETED => + val msg = s"Learningstep with id $learningStepId for learningpath with id $learningPathId not found" + return Failure(NotFoundException(msg)) case None => val msg = s"Learningstep with id $learningStepId for learningpath with id $learningPathId not found" return Failure(NotFoundException(msg)) + case Some(ls) => ls } val (updatedPath, updatedStep) = diff --git a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala index e9aca7342..9762d175c 100644 --- a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala +++ b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala @@ -8,12 +8,9 @@ package no.ndla.learningpathapi.e2e -import io.circe.generic.auto.* -import io.circe.syntax.EncoderOps import no.ndla.common.CirceUtil import no.ndla.common.model.NDLADate import no.ndla.common.model.domain.learningpath.{EmbedType, LearningPath, StepType} -import no.ndla.common.model.domain.myndla.{ArenaGroup, MyNDLAUser, UserRole} import no.ndla.learningpathapi.model.api.{ EmbedUrlV2DTO, LearningPathV2DTO, @@ -23,12 +20,11 @@ import no.ndla.learningpathapi.model.api.{ } import no.ndla.learningpathapi.{ComponentRegistry, LearningpathApiProperties, MainClass, UnitSuite} import no.ndla.scalatestsuite.IntegrationSuite -import org.mockito.ArgumentMatchers.{any, eq as eqTo} -import org.mockito.Mockito.{reset, spy, when, withSettings} +import org.mockito.ArgumentMatchers.any +import org.mockito.Mockito.{spy, when, withSettings} import org.mockito.invocation.InvocationOnMock import org.mockito.quality.Strictness import org.testcontainers.containers.PostgreSQLContainer -import sttp.client3.Response import sttp.client3.quick.* import java.util.concurrent.Executors @@ -185,20 +181,20 @@ class LearningPathAndStepCreationTests } test("That sequence numbers of learningsteps are updated correctly") { - val x = createLearningpath("Test1") - val s1 = createLearningStep(x.id, "Step1") - val s2 = createLearningStep(x.id, "Step2") - val s3 = createLearningStep(x.id, "Step3") - val s4 = createLearningStep(x.id, "Step4") - val s5 = createLearningStep(x.id, "Step5") + val x = createLearningpath("Test1") + val s1 = createLearningStep(x.id, "Step1") + createLearningStep(x.id, "Step2") + createLearningStep(x.id, "Step3") + createLearningStep(x.id, "Step4") + createLearningStep(x.id, "Step5") val pathBeforeDelete = getLearningPath(x.id) pathBeforeDelete.learningsteps.map(_.seqNo) should be(Seq(0, 1, 2, 3, 4)) deleteStep(x.id, s1.id) - deleteStep(x.id, s1.id, None) - deleteStep(x.id, s1.id, None) - deleteStep(x.id, s1.id, None) - deleteStep(x.id, s1.id, None) + deleteStep(x.id, s1.id, Some(404)) + deleteStep(x.id, s1.id, Some(404)) + deleteStep(x.id, s1.id, Some(404)) + deleteStep(x.id, s1.id, Some(404)) val path = getLearningPath(x.id) path.learningsteps.map(_.seqNo) should be(Seq(0, 1, 2, 3)) From 2e629e4ce14d42eb2dbb89c3e9455b198c75eab6 Mon Sep 17 00:00:00 2001 From: Jonas Natten Date: Mon, 3 Feb 2025 07:53:37 +0100 Subject: [PATCH 4/5] learningpath-api: Fix `UpdateServiceTest` after `seqNo` refactor --- .../service/UpdateService.scala | 4 +- .../LearningPathAndStepCreationTests.scala | 10 +- .../service/UpdateServiceTest.scala | 155 +++--------------- 3 files changed, 28 insertions(+), 141 deletions(-) diff --git a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala index b0c0aaaba..fcd8505d0 100644 --- a/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala +++ b/learningpath-api/src/main/scala/no/ndla/learningpathapi/service/UpdateService.scala @@ -312,7 +312,7 @@ trait UpdateService { case ((seqNo, foundStep, steps), curr) => val isChangedStep = curr.id.contains(learningStepId) val (mainStep, stepToAdd) = - if (isChangedStep) (curr, curr.copy(status = newStatus)) + if (isChangedStep) (curr.copy(status = newStatus), curr.copy(status = newStatus)) else (foundStep, curr) val updatedMainStep = mainStep.copy(seqNo = seqNo) val updatedSteps = steps :+ stepToAdd.copy(seqNo = seqNo) @@ -323,7 +323,7 @@ trait UpdateService { val updated = newLearningSteps.map(learningPathRepository.updateLearningStep) val lp = converterService.insertLearningSteps(learningPath, updated, owner) - val updatedPath = learningPathRepository.update(lp) + val updatedPath = learningPathRepository.update(lp.copy(learningsteps = None)) (updatedPath, updatedStep) } diff --git a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala index 9762d175c..1cbf29ef7 100644 --- a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala +++ b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala @@ -36,7 +36,7 @@ class LearningPathAndStepCreationTests extends IntegrationSuite( EnableElasticsearchContainer = true, EnablePostgresContainer = true, - EnableRedisContainer = true + EnableRedisContainer = false ) with UnitSuite { @@ -51,6 +51,7 @@ class LearningPathAndStepCreationTests override def MetaPort: Int = pgc.getMappedPort(5432) override def MetaSchema: String = "testschema" override def disableWarmup: Boolean = true + override def SearchServer: String = elasticSearchHost.get } val someDate: NDLADate = NDLADate.of(2017, 1, 1, 1, 59) @@ -58,9 +59,6 @@ class LearningPathAndStepCreationTests val learningpathApi: MainClass = new MainClass(learningpathApiProperties) { override val componentRegistry: ComponentRegistry = new ComponentRegistry(learningpathApiProperties) { override lazy val clock: SystemClock = mock[SystemClock](withSettings.strictness(Strictness.LENIENT)) -// override lazy val folderRepository: FolderRepository = spy(new FolderRepository) -// override lazy val userRepository: UserRepository = spy(new UserRepository) -// override lazy val userService: UserService = spy(new UserService) override lazy val myndlaApiClient: MyNDLAApiClient = spy(new MyNDLAApiClient) override lazy val taxonomyApiClient: TaxonomyApiClient = mock[TaxonomyApiClient] @@ -81,13 +79,11 @@ class LearningPathAndStepCreationTests super.beforeAll() implicit val ec = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor) Future { learningpathApi.run() }: Unit - Thread.sleep(1000) + Thread.sleep(5000) } override def beforeEach(): Unit = { super.beforeEach() -// reset(learningpathApi.componentRegistry.userRepository) - learningpathApi.componentRegistry.inTransaction(implicit session => { learningpathApi.componentRegistry.learningPathRepository.deleteAllPathsAndSteps(session) }) diff --git a/learningpath-api/src/test/scala/no/ndla/learningpathapi/service/UpdateServiceTest.scala b/learningpath-api/src/test/scala/no/ndla/learningpathapi/service/UpdateServiceTest.scala index 5620e2caa..3cf1cad76 100644 --- a/learningpath-api/src/test/scala/no/ndla/learningpathapi/service/UpdateServiceTest.scala +++ b/learningpath-api/src/test/scala/no/ndla/learningpathapi/service/UpdateServiceTest.scala @@ -787,8 +787,8 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { test("That updateLearningStepStatusV2 returns None when the given learningstep does not exist") { when(learningPathRepository.withId(eqTo(PUBLISHED_ID))(any[DBSession])) .thenReturn(Some(PUBLISHED_LEARNINGPATH)) - when(learningPathRepository.learningStepWithId(eqTo(PUBLISHED_ID), eqTo(STEP1.id.get))(any[DBSession])) - .thenReturn(None) + when(learningPathRepository.learningStepsFor(eqTo(PUBLISHED_ID))(any[DBSession])) + .thenReturn(Seq()) val Failure(ex) = service.updateLearningStepStatusV2(PUBLISHED_ID, STEP1.id.get, StepStatus.DELETED, PUBLISHED_OWNER.toCombined) @@ -803,7 +803,7 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { when(learningPathRepository.learningStepWithId(eqTo(PRIVATE_ID), eqTo(STEP1.id.get))(any[DBSession])) .thenReturn(Some(STEP1)) when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(List()) + .thenReturn(List(STEP1)) when(learningPathRepository.updateLearningStep(eqTo(STEP1.copy(status = StepStatus.DELETED)))(any[DBSession])) .thenReturn(STEP1.copy(status = StepStatus.DELETED)) when(learningPathRepository.update(any[LearningPath])(any[DBSession])) @@ -829,9 +829,11 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { .thenReturn(Some(PUBLISHED_LEARNINGPATH)) when(learningPathRepository.learningStepWithId(eqTo(PUBLISHED_ID), eqTo(STEP1.id.get))(any[DBSession])) .thenReturn(Some(STEP1)) - when(learningPathRepository.learningStepsFor(eqTo(PUBLISHED_ID))(any[DBSession])).thenReturn(List()) - when(learningPathRepository.updateLearningStep(eqTo(STEP1.copy(status = StepStatus.DELETED)))(any[DBSession])) - .thenReturn(STEP1.copy(status = StepStatus.DELETED)) + when(learningPathRepository.learningStepsFor(eqTo(PUBLISHED_ID))(any[DBSession])) + .thenReturn(PUBLISHED_LEARNINGPATH.learningsteps.get) + when(learningPathRepository.updateLearningStep(any)(any)).thenAnswer((i: InvocationOnMock) => + i.getArgument[LearningStep](0) + ) when(learningPathRepository.update(any[LearningPath])(any[DBSession])).thenAnswer((i: InvocationOnMock) => i.getArgument[LearningPath](0) ) @@ -844,17 +846,16 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { updatedStep.isSuccess should be(true) updatedStep.get.status should equal(StepStatus.DELETED.entryName) + val expectedUpdatePath = + PUBLISHED_LEARNINGPATH.copy( + learningsteps = None, + status = LearningPathStatus.UNLISTED, + lastUpdated = updatedDate + ) + verify(learningPathRepository, times(1)) .updateLearningStep(eqTo(STEP1.copy(status = StepStatus.DELETED)))(any[DBSession]) - verify(learningPathRepository, times(1)).update( - eqTo( - PUBLISHED_LEARNINGPATH.copy( - learningsteps = PUBLISHED_LEARNINGPATH.learningsteps, - status = LearningPathStatus.UNLISTED, - lastUpdated = updatedDate - ) - ) - )(any[DBSession]) + verify(learningPathRepository, times(1)).update(eqTo(expectedUpdatePath))(any[DBSession]) verify(searchIndexService, times(1)).indexDocument(any[LearningPath]) verify(searchIndexService, times(0)).deleteDocument(any[LearningPath], any) } @@ -866,8 +867,8 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { .thenReturn(Some(STEP1)) when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) .thenReturn(List(STEP1, STEP2, STEP3)) - when(learningPathRepository.updateLearningStep(eqTo(STEP1.copy(status = StepStatus.DELETED)))(any[DBSession])) - .thenReturn(STEP1.copy(status = StepStatus.DELETED)) + when(learningPathRepository.updateLearningStep(any)(any[DBSession])) + .thenAnswer((i: InvocationOnMock) => i.getArgument[LearningStep](0)) when(learningPathRepository.update(any[LearningPath])(any[DBSession])) .thenReturn(PRIVATE_LEARNINGPATH) when(learningPathRepository.learningPathsWithIsBasedOn(any[Long])).thenReturn(List.empty) @@ -898,8 +899,8 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { .thenReturn(Some(STEP1.copy(status = StepStatus.DELETED))) when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) .thenReturn(List(STEP1, STEP2, STEP3)) - when(learningPathRepository.updateLearningStep(eqTo(STEP1.copy(status = StepStatus.ACTIVE)))(any[DBSession])) - .thenReturn(STEP1.copy(status = StepStatus.ACTIVE)) + when(learningPathRepository.updateLearningStep(any)(any[DBSession])) + .thenAnswer((i: InvocationOnMock) => i.getArgument[LearningStep](0)) when(learningPathRepository.update(any[LearningPath])(any[DBSession])) .thenReturn(PRIVATE_LEARNINGPATH) when(learningPathRepository.learningPathsWithIsBasedOn(any[Long])).thenReturn(List.empty) @@ -910,121 +911,11 @@ class UpdateServiceTest extends UnitSuite with UnitTestEnvironment { updatedStep.get.status should equal(StepStatus.ACTIVE.entryName) verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP1.copy(status = StepStatus.ACTIVE)))(any[DBSession]) - verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP2.copy(seqNo = STEP2.seqNo + 1)))(any[DBSession]) - verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP3.copy(seqNo = STEP3.seqNo + 1)))(any[DBSession]) - verify(learningPathRepository, times(1)) - .update(any[LearningPath])(any[DBSession]) - verify(searchIndexService, times(1)).indexDocument(any[LearningPath]) - } - - test("That marking the last learningStep as deleted does not affect any of the other learningsteps") { - when(learningPathRepository.withId(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(Some(PRIVATE_LEARNINGPATH)) - when(learningPathRepository.learningStepWithId(eqTo(PRIVATE_ID), eqTo(STEP3.id.get))(any[DBSession])) - .thenReturn(Some(STEP3)) - when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(List(STEP1, STEP2, STEP3)) - when(learningPathRepository.updateLearningStep(eqTo(STEP3.copy(status = StepStatus.DELETED)))(any[DBSession])) - .thenReturn(STEP3.copy(status = StepStatus.DELETED)) - when(learningPathRepository.update(any[LearningPath])(any[DBSession])) - .thenReturn(PRIVATE_LEARNINGPATH) - when(learningPathRepository.learningPathsWithIsBasedOn(any[Long])).thenReturn(List.empty) - - val updatedStep = - service.updateLearningStepStatusV2(PRIVATE_ID, STEP3.id.get, StepStatus.DELETED, PRIVATE_OWNER.toCombined) - updatedStep.isSuccess should be(true) - updatedStep.get.status should equal(StepStatus.DELETED.entryName) - + .updateLearningStep(eqTo(STEP1.copy(status = StepStatus.ACTIVE, seqNo = 0)))(any[DBSession]) verify(learningPathRepository, times(1)) - .updateLearningStep(any[LearningStep])(any[DBSession]) + .updateLearningStep(eqTo(STEP2.copy(seqNo = 1)))(any[DBSession]) verify(learningPathRepository, times(1)) - .update(any[LearningPath])(any[DBSession]) - verify(searchIndexService, times(1)).indexDocument(any[LearningPath]) - } - - test("That marking the last learningStep as active does not affect any of the other learningsteps") { - when(learningPathRepository.withId(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(Some(PRIVATE_LEARNINGPATH)) - when( - learningPathRepository - .learningStepWithId(eqTo(PRIVATE_ID), eqTo(STEP3.id.get))(any[DBSession]) - ) - .thenReturn(Some(STEP3.copy(status = StepStatus.DELETED))) - when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(List(STEP1, STEP2, STEP3)) - when(learningPathRepository.updateLearningStep(eqTo(STEP3.copy(status = StepStatus.ACTIVE)))(any[DBSession])) - .thenReturn(STEP3.copy(status = StepStatus.ACTIVE)) - when(learningPathRepository.update(any[LearningPath])(any[DBSession])) - .thenReturn(PRIVATE_LEARNINGPATH) - when(learningPathRepository.learningPathsWithIsBasedOn(any[Long])).thenReturn(List.empty) - - val updatedStep = - service.updateLearningStepStatusV2(PRIVATE_ID, STEP3.id.get, StepStatus.ACTIVE, PRIVATE_OWNER.toCombined) - updatedStep.isSuccess should be(true) - updatedStep.get.status should equal(StepStatus.ACTIVE.entryName) - - verify(learningPathRepository, times(1)) - .updateLearningStep(any[LearningStep])(any[DBSession]) - verify(learningPathRepository, times(1)) - .update(any[LearningPath])(any[DBSession]) - verify(searchIndexService, times(1)).indexDocument(any[LearningPath]) - } - - test("That marking the middle learningStep as deleted only affects subsequent learningsteps") { - when(learningPathRepository.withId(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(Some(PRIVATE_LEARNINGPATH)) - when(learningPathRepository.learningStepWithId(eqTo(PRIVATE_ID), eqTo(STEP2.id.get))(any[DBSession])) - .thenReturn(Some(STEP2)) - when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(List(STEP1, STEP2, STEP3)) - when(learningPathRepository.updateLearningStep(eqTo(STEP2.copy(status = StepStatus.DELETED)))(any[DBSession])) - .thenReturn(STEP2.copy(status = StepStatus.DELETED)) - when(learningPathRepository.update(any[LearningPath])(any[DBSession])) - .thenReturn(PRIVATE_LEARNINGPATH) - when(learningPathRepository.learningPathsWithIsBasedOn(any[Long])).thenReturn(List.empty) - - val updatedStep = - service.updateLearningStepStatusV2(PRIVATE_ID, STEP2.id.get, StepStatus.DELETED, PRIVATE_OWNER.toCombined) - updatedStep.isSuccess should be(true) - updatedStep.get.status should equal(StepStatus.DELETED.entryName) - - verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP2.copy(status = StepStatus.DELETED)))(any[DBSession]) - verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP3.copy(seqNo = STEP3.seqNo - 1)))(any[DBSession]) - verify(learningPathRepository, times(1)) - .update(any[LearningPath])(any[DBSession]) - verify(searchIndexService, times(1)).indexDocument(any[LearningPath]) - } - - test("That marking the middle learningStep as active only affects subsequent learningsteps") { - when(learningPathRepository.withId(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(Some(PRIVATE_LEARNINGPATH)) - when( - learningPathRepository - .learningStepWithId(eqTo(PRIVATE_ID), eqTo(STEP2.id.get))(any[DBSession]) - ) - .thenReturn(Some(STEP2.copy(status = StepStatus.DELETED))) - when(learningPathRepository.learningStepsFor(eqTo(PRIVATE_ID))(any[DBSession])) - .thenReturn(List(STEP1, STEP2, STEP3)) - when(learningPathRepository.updateLearningStep(eqTo(STEP2.copy(status = StepStatus.ACTIVE)))(any[DBSession])) - .thenReturn(STEP2.copy(status = StepStatus.ACTIVE)) - when(learningPathRepository.update(any[LearningPath])(any[DBSession])) - .thenReturn(PRIVATE_LEARNINGPATH) - when(learningPathRepository.learningPathsWithIsBasedOn(any[Long])).thenReturn(List.empty) - - val updatedStep = - service.updateLearningStepStatusV2(PRIVATE_ID, STEP2.id.get, StepStatus.ACTIVE, PRIVATE_OWNER.toCombined) - updatedStep.isSuccess should be(true) - updatedStep.get.status should equal(StepStatus.ACTIVE.entryName) - - verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP2.copy(status = StepStatus.ACTIVE)))(any[DBSession]) - verify(learningPathRepository, times(1)) - .updateLearningStep(eqTo(STEP3.copy(seqNo = STEP3.seqNo + 1)))(any[DBSession]) + .updateLearningStep(eqTo(STEP3.copy(seqNo = 2)))(any[DBSession]) verify(learningPathRepository, times(1)) .update(any[LearningPath])(any[DBSession]) verify(searchIndexService, times(1)).indexDocument(any[LearningPath]) From a7ebb883c25fe96c3a642f81b3d10f159e229bea Mon Sep 17 00:00:00 2001 From: Jonas Natten Date: Mon, 3 Feb 2025 08:39:16 +0100 Subject: [PATCH 5/5] Update learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala Co-authored-by: Gunnar Velle --- .../learningpathapi/e2e/LearningPathAndStepCreationTests.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala index 1cbf29ef7..0c090bb27 100644 --- a/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala +++ b/learningpath-api/src/test/scala/no/ndla/learningpathapi/e2e/LearningPathAndStepCreationTests.scala @@ -1,5 +1,5 @@ /* - * Part of NDLA backend.learningpath-api.test + * Part of NDLA learningpath-api * Copyright (C) 2025 NDLA * * See LICENSE