diff --git a/common/src/main/scala/com/gu/sfl/persistence/SavedArticlesPersistence.scala b/common/src/main/scala/com/gu/sfl/persistence/SavedArticlesPersistence.scala index 747ef8f4..e729e28a 100644 --- a/common/src/main/scala/com/gu/sfl/persistence/SavedArticlesPersistence.scala +++ b/common/src/main/scala/com/gu/sfl/persistence/SavedArticlesPersistence.scala @@ -29,11 +29,6 @@ trait SavedArticlesPersistence { userId: String, savedArticles: SavedArticles ): Try[Option[SavedArticles]] - - def write( - userId: String, - savedArticles: SavedArticles - ): Try[Option[SavedArticles]] } case class DynamoSavedArticles( @@ -119,32 +114,6 @@ class SavedArticlesPersistenceImpl(persistanceConfig: PersistenceConfig) } } - override def write( - userId: String, - savedArticles: SavedArticles - ): Try[Option[SavedArticles]] = { - scanamo.exec( - table.putAndReturn(PutReturn.OldValue)( - DynamoSavedArticles(userId, savedArticles) - ) - ) match { - case Some(Right(articles)) => - logger.debug(s"Succcesfully saved articles for $userId") - Success(Some(articles.ordered)) - case Some(Left(error)) => - val exception = new IllegalArgumentException(s"$error") - logger.error( - s"Exception Thrown saving articles for $userId:", - exception - ) - Failure(exception) - case None => { - logger.debug(s"Successfully saved but none retrieved for $userId") - Success(Some(savedArticles)) - } - } - } - override def update( userId: String, savedArticles: SavedArticles diff --git a/mobile-save-for-later/src/test/scala/com/gu/sfl/lib/ArticleMergeSpecification.scala b/mobile-save-for-later/src/test/scala/com/gu/sfl/lib/ArticleMergeSpecification.scala index 3cdb6089..ee6e35a0 100644 --- a/mobile-save-for-later/src/test/scala/com/gu/sfl/lib/ArticleMergeSpecification.scala +++ b/mobile-save-for-later/src/test/scala/com/gu/sfl/lib/ArticleMergeSpecification.scala @@ -11,38 +11,69 @@ import org.specs2.specification.Scope import scala.util.{Failure, Success} -class ArticleMergeSpecification extends Specification with Mockito { +class ArticleMergeSpecification extends Specification with Mockito { "updateSavedArticlesWithRetryAndMerge" should { val userId = "123" val version = "1" val (article1, article2, article3, article4) = ( - SavedArticle("id/1", "p/1", LocalDateTime.of(2018, 1, 16, 16, 30), read = true), - SavedArticle("id/2", "p/2", LocalDateTime.of(2018, 2, 17, 17, 30), read = false), - SavedArticle("id/3", "p/3", LocalDateTime.of(2018, 3, 18, 18, 30), read = true), - SavedArticle("id/4", "p/4", LocalDateTime.of(2018, 4, 19, 19, 30), read = true) + SavedArticle( + "id/1", + "p/1", + LocalDateTime.of(2018, 1, 16, 16, 30), + read = true + ), + SavedArticle( + "id/2", + "p/2", + LocalDateTime.of(2018, 2, 17, 17, 30), + read = false + ), + SavedArticle( + "id/3", + "p/3", + LocalDateTime.of(2018, 3, 18, 18, 30), + read = true + ), + SavedArticle( + "id/4", + "p/4", + LocalDateTime.of(2018, 4, 19, 19, 30), + read = true + ) ) - val savedArticles = SavedArticles(version, List(article1, article2)) val savedArticlesUpdate1 = SavedArticles(version, List(article3)) - val savedArticles2 = SavedArticles(version, List(article1, article2, article3)) - val article1Dup = SavedArticle("id/1", "p/1", LocalDateTime.of(2017, 1, 16, 16, 30), read = true) - val article2Dup = SavedArticle("id/2", "p/2", LocalDateTime.of(2017, 3, 17, 17, 30), read = false) - + val savedArticles2 = + SavedArticles(version, List(article1, article2, article3)) + val article1Dup = SavedArticle( + "id/1", + "p/1", + LocalDateTime.of(2017, 1, 16, 16, 30), + read = true + ) + val article2Dup = SavedArticle( + "id/2", + "p/2", + LocalDateTime.of(2017, 3, 17, 17, 30), + read = false + ) "saves the articles if the user does not currently have any articles saved" in new Setup { val responseArticles = Success(Some(savedArticles.advanceVersion)) val expectedMergeResponse = Right(savedArticles.advanceVersion) savedArticlesPersistence.read(userId) returns (Success(None)) - savedArticlesPersistence.write(userId, savedArticles) returns (responseArticles) - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) + savedArticlesPersistence.update( + userId, + savedArticles + ) returns (responseArticles) + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) there was one(savedArticlesPersistence).read(userId) - - there was no(savedArticlesPersistence).update(Mockito.any[String](), Mockito.any[SavedArticles]()) - there were no(savedArticlesPersistence).update(userId, savedArticles) + there was one(savedArticlesPersistence).update(userId, savedArticles) saved shouldEqual (expectedMergeResponse) } @@ -50,116 +81,222 @@ class ArticleMergeSpecification extends Specification with Mockito { val responseArticles = Success(Some(savedArticles2.advanceVersion)) val expectedMergeResponse = Right(savedArticles2.advanceVersion) - savedArticlesPersistence.read(userId) returns(Success(Some(savedArticles))) - savedArticlesPersistence.update(argThat(===(userId)), argThat(===(savedArticles2))) returns(responseArticles) - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles2) + savedArticlesPersistence.read(userId) returns (Success( + Some(savedArticles) + )) + savedArticlesPersistence.update( + argThat(===(userId)), + argThat(===(savedArticles2)) + ) returns (responseArticles) + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles2) there was one(savedArticlesPersistence).read(argThat(===(userId))) - there were no(savedArticlesPersistence).write(any[String](), any[SavedArticles]()) - there was one(savedArticlesPersistence).update(argThat(===(userId)), argThat(===(savedArticles2))) + there were no(savedArticlesPersistence).write( + any[String](), + any[SavedArticles]() + ) + there was one(savedArticlesPersistence).update( + argThat(===(userId)), + argThat(===(savedArticles2)) + ) saved shouldEqual (expectedMergeResponse) } "will not persist saved articles if they are identical to the articles currently saved and there is no conflict" in new Setup { - savedArticlesPersistence.read(userId) returns(Success(Some(savedArticles))) + savedArticlesPersistence.read(userId) returns (Success( + Some(savedArticles) + )) - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) there was one(savedArticlesPersistence).read(argThat(===(userId))) - there were no(savedArticlesPersistence).write(any[String](), any[SavedArticles]()) - there were no(savedArticlesPersistence).update(any[String](), any[SavedArticles]()) + there were no(savedArticlesPersistence).write( + any[String](), + any[SavedArticles]() + ) + there were no(savedArticlesPersistence).update( + any[String](), + any[SavedArticles]() + ) - saved shouldEqual(Right(savedArticles)) + saved shouldEqual (Right(savedArticles)) } "will merge articles if there is a cnnflict" in new Setup { val articlesCurrentlySaved = SavedArticles("2", List(article1, article2)) - val articlesToSave = SavedArticles("1", List(article1, article2, article3)) - val expectedMergedArticles = SavedArticles("2", List(article1, article2, article3)) - savedArticlesPersistence.read(userId) returns(Success(Some(articlesCurrentlySaved))) - savedArticlesPersistence.update(any[String](), any[SavedArticles]()) returns(Success(Some(expectedMergedArticles.advanceVersion))) + val articlesToSave = + SavedArticles("1", List(article1, article2, article3)) + val expectedMergedArticles = + SavedArticles("2", List(article1, article2, article3)) + savedArticlesPersistence.read(userId) returns (Success( + Some(articlesCurrentlySaved) + )) + savedArticlesPersistence.update( + any[String](), + any[SavedArticles]() + ) returns (Success(Some(expectedMergedArticles.advanceVersion))) savedArticlesMerger.updateWithRetryAndMerge(userId, articlesToSave) - there was one(savedArticlesPersistence).update(argThat(===(userId)), argThat(===(expectedMergedArticles))) + there was one(savedArticlesPersistence).update( + argThat(===(userId)), + argThat(===(expectedMergedArticles)) + ) } - + "will dedupe merged articles if there is a conflict" in new Setup { - val articlesCurrentlySaved = SavedArticles("2", List(article1Dup, article2)) - val articlesToSave = SavedArticles("1", List(article1, article2, article3)) - val expectedMergedArticles = SavedArticles("2", List(article1, article2, article3)) - savedArticlesPersistence.read(userId) returns(Success(Some(articlesCurrentlySaved))) - savedArticlesPersistence.update(any[String](), any[SavedArticles]()) returns(Success(Some(expectedMergedArticles.advanceVersion))) + val articlesCurrentlySaved = + SavedArticles("2", List(article1Dup, article2)) + val articlesToSave = + SavedArticles("1", List(article1, article2, article3)) + val expectedMergedArticles = + SavedArticles("2", List(article1, article2, article3)) + savedArticlesPersistence.read(userId) returns (Success( + Some(articlesCurrentlySaved) + )) + savedArticlesPersistence.update( + any[String](), + any[SavedArticles]() + ) returns (Success(Some(expectedMergedArticles.advanceVersion))) savedArticlesMerger.updateWithRetryAndMerge(userId, articlesToSave) - there was one(savedArticlesPersistence).update(argThat(===(userId)), argThat(===(expectedMergedArticles))) + there was one(savedArticlesPersistence).update( + argThat(===(userId)), + argThat(===(expectedMergedArticles)) + ) } "will select the latest articles up to the limit where the article limit is broken and there is no conflict`" in new Setup { private val maxSavedArticlesLimit = 2 - override val savedArticlesMerger = new SavedArticlesMergerImpl(SavedArticlesMergerConfig(maxSavedArticlesLimit), savedArticlesPersistence) - val expectedArticlesPersisted = savedArticles.copy(articles = List(article2, article3)) - val expectedMergeResponse = Right(expectedArticlesPersisted.advanceVersion) - savedArticlesPersistence.update(any[String](), any[SavedArticles]()) returns(Success(Some(expectedArticlesPersisted.advanceVersion))) - savedArticlesPersistence.read(userId) returns(Success(Some(savedArticles))) - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles2) - - there were no (savedArticlesPersistence).write(any[String](), any[SavedArticles]()) - there was one (savedArticlesPersistence).update(argThat(===(userId)), argThat(===(expectedArticlesPersisted))) - saved mustEqual(expectedMergeResponse) + override val savedArticlesMerger = new SavedArticlesMergerImpl( + SavedArticlesMergerConfig(maxSavedArticlesLimit), + savedArticlesPersistence + ) + val expectedArticlesPersisted = + savedArticles.copy(articles = List(article2, article3)) + val expectedMergeResponse = + Right(expectedArticlesPersisted.advanceVersion) + savedArticlesPersistence.update( + any[String](), + any[SavedArticles]() + ) returns (Success(Some(expectedArticlesPersisted.advanceVersion))) + savedArticlesPersistence.read(userId) returns (Success( + Some(savedArticles) + )) + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles2) + + there were no(savedArticlesPersistence).write( + any[String](), + any[SavedArticles]() + ) + there was one(savedArticlesPersistence).update( + argThat(===(userId)), + argThat(===(expectedArticlesPersisted)) + ) + saved mustEqual (expectedMergeResponse) } "will select the latest articles up to the limit where the article limit is broken and there is a conflict" in new Setup { private val maxSavedArticlesLimit = 2 - override val savedArticlesMerger = new SavedArticlesMergerImpl(SavedArticlesMergerConfig(maxSavedArticlesLimit), savedArticlesPersistence) + override val savedArticlesMerger = new SavedArticlesMergerImpl( + SavedArticlesMergerConfig(maxSavedArticlesLimit), + savedArticlesPersistence + ) val articlesCurrentlySaved = SavedArticles("2", List(article1, article2)) - val articlesToSave = SavedArticles("1", List(article1, article2, article3)) + val articlesToSave = + SavedArticles("1", List(article1, article2, article3)) //Returns an object consisting of the version currently saved and the latest 2 of th - val expectedArticlesPersisted = SavedArticles("2", List(article2, article3)) - val expectedMergeResponse = Right(expectedArticlesPersisted.advanceVersion) - - savedArticlesPersistence.read(any[String]()) returns(Success(Some(articlesCurrentlySaved))) - savedArticlesPersistence.update(any[String](), any[SavedArticles]()) returns(Success(Some(expectedArticlesPersisted.advanceVersion))) - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, articlesToSave) - there were no (savedArticlesPersistence).write(any[String](), any[SavedArticles]()) - there was one (savedArticlesPersistence).update(argThat(===(userId)), argThat(===(expectedArticlesPersisted))) - saved mustEqual(expectedMergeResponse) + val expectedArticlesPersisted = + SavedArticles("2", List(article2, article3)) + val expectedMergeResponse = + Right(expectedArticlesPersisted.advanceVersion) + + savedArticlesPersistence.read(any[String]()) returns (Success( + Some(articlesCurrentlySaved) + )) + savedArticlesPersistence.update( + any[String](), + any[SavedArticles]() + ) returns (Success(Some(expectedArticlesPersisted.advanceVersion))) + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, articlesToSave) + there were no(savedArticlesPersistence).write( + any[String](), + any[SavedArticles]() + ) + there was one(savedArticlesPersistence).update( + argThat(===(userId)), + argThat(===(expectedArticlesPersisted)) + ) + saved mustEqual (expectedMergeResponse) } "will dedupe articles before checking whether the article limit hs been transgressed" in new Setup { private val maxSavedArticlesLimit = 3 - override val savedArticlesMerger = new SavedArticlesMergerImpl(SavedArticlesMergerConfig(maxSavedArticlesLimit), savedArticlesPersistence) - val savedArticlesWithDupes = savedArticles2.copy(articles = List(article1, article1Dup, article2, article2Dup)) + override val savedArticlesMerger = new SavedArticlesMergerImpl( + SavedArticlesMergerConfig(maxSavedArticlesLimit), + savedArticlesPersistence + ) + val savedArticlesWithDupes = savedArticles2.copy(articles = + List(article1, article1Dup, article2, article2Dup) + ) val responseArticles = Success(Some(savedArticles.advanceVersion)) val expectedMergeResponse = Right(savedArticles.advanceVersion) val expectedDeduped = SavedArticles(version, List(article1, article2)) savedArticlesPersistence.read(userId) returns (Success(None)) - savedArticlesPersistence.write(any[String](), any[SavedArticles]()) returns (responseArticles) - savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticlesWithDupes) + savedArticlesPersistence.update( + any[String](), + any[SavedArticles]() + ) returns (responseArticles) + savedArticlesMerger.updateWithRetryAndMerge( + userId, + savedArticlesWithDupes + ) there was one(savedArticlesPersistence).read(argThat(===(userId))) - there was one(savedArticlesPersistence).write(argThat(===(userId)), argThat(===(expectedDeduped))) + there was one(savedArticlesPersistence).update( + argThat(===(userId)), + argThat(===(expectedDeduped)) + ) } "failure to get current articles throws the correct exception" in new Setup { - savedArticlesPersistence.read(userId) returns (Failure(new IllegalStateException("Bad, bad, bad"))) - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) + savedArticlesPersistence.read(userId) returns (Failure( + new IllegalStateException("Bad, bad, bad") + )) + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) there was one(savedArticlesPersistence).read(argThat(===(userId))) - saved shouldEqual (Left(SavedArticleMergeError("Could not retrieve current articles"))) + saved shouldEqual (Left( + SavedArticleMergeError("Could not retrieve current articles") + )) } "failure to update the saved articles results in the currect error" in new Setup { savedArticlesPersistence.read(userId) returns (Success(None)) - savedArticlesPersistence.write(userId, savedArticles) returns (Failure(new IllegalStateException("My mummy told me to be good, but I was naughty"))) - - val saved = savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) - saved shouldEqual (Left(SavedArticleMergeError("Could not update articles"))) + savedArticlesPersistence.update(userId, savedArticles) returns (Failure( + new IllegalStateException( + "My mummy told me to be good, but I was naughty" + ) + )) + + val saved = + savedArticlesMerger.updateWithRetryAndMerge(userId, savedArticles) + saved shouldEqual (Left( + SavedArticleMergeError("Could not update articles") + )) } } trait Setup extends Scope { val savedArticlesPersistence = mock[SavedArticlesPersistenceImpl] - val savedArticlesMerger = new SavedArticlesMergerImpl(SavedArticlesMergerConfig(20), savedArticlesPersistence) + val savedArticlesMerger = new SavedArticlesMergerImpl( + SavedArticlesMergerConfig(20), + savedArticlesPersistence + ) } } diff --git a/project/dependencies.scala b/project/dependencies.scala index bf36ab43..898ca1c2 100644 --- a/project/dependencies.scala +++ b/project/dependencies.scala @@ -4,7 +4,7 @@ object Dependencies { val awsSdkVersion = "1.11.412" val log4j2Version = "2.17.1" val jacksonVersion = "2.14.0" - val specsVersion = "4.0.3" + val specsVersion = "4.20.5" val awsLambda = "com.amazonaws" % "aws-lambda-java-core" % "1.2.0" val awsDynamo = "software.amazon.awssdk" % "dynamodb" % "2.24.11"