From cb6cb1de7830d665877a2e67df5c347d151e6624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B3bert=20Papp?= Date: Sat, 13 Jan 2024 21:53:28 +0000 Subject: [PATCH] Reduce model and result coupling (#72) --- .../controller/LineStatusHistoryController.kt | 14 ++-- .../travel/statushistory/viewmodel/Changes.kt | 2 +- .../travel/statushistory/viewmodel/Result.kt | 5 +- .../viewmodel/ResultChangeModel.kt | 57 +++++++++++---- .../viewmodel/ResultChangeModelMapper.kt | 71 ++++++++++--------- .../viewmodel/ResultChangesCalculator.kt | 2 +- .../src/main/resources/views/LineStatus.hbs | 47 ++++++------ ...hangeCalculatorUnitTest_NonContentDiffs.kt | 2 +- .../viewmodel/ResultChangeModelUnitTest.kt | 26 ------- .../ResultErrorResultErrorUnitTest.kt | 6 +- 10 files changed, 117 insertions(+), 115 deletions(-) delete mode 100644 web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelUnitTest.kt diff --git a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/controller/LineStatusHistoryController.kt b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/controller/LineStatusHistoryController.kt index b829928f..bd615c49 100644 --- a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/controller/LineStatusHistoryController.kt +++ b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/controller/LineStatusHistoryController.kt @@ -10,13 +10,13 @@ import io.micronaut.views.View import net.twisterrob.blt.data.StaticData import net.twisterrob.blt.io.feeds.trackernet.LineStatusFeed import net.twisterrob.travel.domain.london.status.Feed -import net.twisterrob.travel.domain.london.status.api.StatusHistoryRepository import net.twisterrob.travel.domain.london.status.api.ParsedStatusItem +import net.twisterrob.travel.domain.london.status.api.StatusHistoryRepository import net.twisterrob.travel.statushistory.viewmodel.LineColor +import net.twisterrob.travel.statushistory.viewmodel.LineStatusModel import net.twisterrob.travel.statushistory.viewmodel.Result -import net.twisterrob.travel.statushistory.viewmodel.ResultChangesCalculator -import net.twisterrob.travel.statushistory.viewmodel.ResultChangeModel import net.twisterrob.travel.statushistory.viewmodel.ResultChangeModelMapper +import net.twisterrob.travel.statushistory.viewmodel.ResultChangesCalculator import java.util.Date @Controller @@ -41,18 +41,12 @@ class LineStatusHistoryController( val changes = ResultChangesCalculator().getChanges(results) return HttpResponse.ok( - LineStatusHistoryModel( + LineStatusModel( changes.map(ResultChangeModelMapper()::map), LineColor.AllColors(staticData.lineColors) ) ) } - - @Suppress("unused") // Used by LineStatus.hbs. - private class LineStatusHistoryModel( - val feedChanges: List, - val colors: Iterable, - ) } private fun ParsedStatusItem.toResult(): Result { diff --git a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Changes.kt b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Changes.kt index d22fcf92..a2e392a2 100644 --- a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Changes.kt +++ b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Changes.kt @@ -6,7 +6,7 @@ import net.twisterrob.blt.model.Line sealed interface Changes { - data object None : Changes + data object Inconclusive : Changes data class NewStatus( val current: Result, diff --git a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Result.kt b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Result.kt index f2ccce0b..f8debb87 100644 --- a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Result.kt +++ b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/Result.kt @@ -5,7 +5,6 @@ import java.util.Date sealed interface Result { - // Used by Handlebars reflection in LineStatus.hbs. @Suppress("detekt.VariableNaming") val `when`: Date @@ -21,11 +20,11 @@ sealed interface Result { @JvmInline value class Error( - val error: String, + val text: String, ) { val header: String - get() = error.substringBefore('\n') + get() = text.substringBefore('\n') } } } diff --git a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModel.kt b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModel.kt index ed5bc86f..f82e7c55 100644 --- a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModel.kt +++ b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModel.kt @@ -1,15 +1,32 @@ +@file:Suppress("unused", "CanBeParameter", "MemberVisibilityCanBePrivate") // Used by LineStatus.hbs. + package net.twisterrob.travel.statushistory.viewmodel +import net.twisterrob.blt.io.feeds.trackernet.model.DelayType import net.twisterrob.blt.model.Line +import java.util.Date + +class LineStatusModel( + val feedChanges: List, + val colors: Iterable, +) -open class ResultChangeModel( - val previous: Result?, - val current: Result?, +class ResultChangeModel( + val `when`: Date?, + val statuses: List, val error: ErrorChange?, - val statuses: Map, - val descriptions: Map, ) { + class LineStatusModel( + val line: Line, + val type: DelayType, + val description: String?, + val active: Boolean, + val branchDescription: String?, + val changeStatus: StatusChange?, + val changeDescription: String?, + ) + enum class StatusChange( val title: String, val cssClass: String, @@ -24,18 +41,30 @@ open class ResultChangeModel( SameDescriptionAdd("+ descr.", "status-same-desc-add"), SameDescriptionDel("- descr.", "status-same-desc-del"), BranchesChange("branches", "status-same-branch-change"), + ; + + init { + require(cssClass.isNotEmpty()) { "CSS class must not be empty: ${this} has cssClass=${cssClass}." } + } } - enum class ErrorChange( - val title: String, + class ErrorChange( + val type: Type, + val header: String?, + val full: String?, ) { - Same("same error"), - Change("error changed"), - Failed("new error"), - Fixed("error fixed"), - NoErrors(""), - NewStatus(""), - LastStatus(""), + enum class Type( + val title: String, + ) { + + Same("same error"), + Change("error changed"), + Failed("new error"), + Fixed("error fixed"), + NoErrors(""), + NewStatus(""), + LastStatus(""), + } } } diff --git a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelMapper.kt b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelMapper.kt index 1a0a8b79..c68c59f0 100644 --- a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelMapper.kt +++ b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelMapper.kt @@ -2,33 +2,40 @@ package net.twisterrob.travel.statushistory.viewmodel import net.twisterrob.blt.diff.HtmlDiff import net.twisterrob.blt.io.feeds.trackernet.model.LineStatus +import net.twisterrob.blt.model.Line +import net.twisterrob.travel.statushistory.viewmodel.ResultChangeModel.LineStatusModel class ResultChangeModelMapper { - fun map(changes: Changes): ResultChangeModel { - val statusChanges = (changes as? Changes.Status)?.changes.orEmpty() - return ResultChangeModel( - previous = changes.previous, - current = changes.current, + fun map(changes: Changes): ResultChangeModel = + ResultChangeModel( + `when` = changes.current?.`when`, error = mapError(changes), - statuses = statusChanges.mapValues { map(it.value) }, - descriptions = statusChanges - .filterValues { it is HasDescriptionChange } - .mapValues { it.value as HasDescriptionChange } - .mapValues { diffDesc(it.value.desc) }, + statuses = map( + (changes.current as? Result.ContentResult)?.content?.lineStatuses.orEmpty(), + (changes as? Changes.Status)?.changes.orEmpty(), + ), + ) + + private fun mapError(changes: Changes): ResultChangeModel.ErrorChange { + val error = (changes.current as? Result.ErrorResult)?.error + return ResultChangeModel.ErrorChange( + full = error?.text, + header = error?.header, + type = mapErrorType(changes), ) } - private fun mapError(changes: Changes): ResultChangeModel.ErrorChange = + private fun mapErrorType(changes: Changes): ResultChangeModel.ErrorChange.Type = when (changes) { - is Changes.Status -> ResultChangeModel.ErrorChange.NoErrors - Changes.None -> ResultChangeModel.ErrorChange.NoErrors - is Changes.NewStatus -> ResultChangeModel.ErrorChange.NewStatus - is Changes.LastStatus -> ResultChangeModel.ErrorChange.LastStatus - is Changes.ErrorChanges.Same -> ResultChangeModel.ErrorChange.Same - is Changes.ErrorChanges.Change -> ResultChangeModel.ErrorChange.Change - is Changes.ErrorChanges.Failed -> ResultChangeModel.ErrorChange.Failed - is Changes.ErrorChanges.Fixed -> ResultChangeModel.ErrorChange.Fixed + is Changes.Status -> ResultChangeModel.ErrorChange.Type.NoErrors + Changes.Inconclusive -> ResultChangeModel.ErrorChange.Type.NoErrors + is Changes.NewStatus -> ResultChangeModel.ErrorChange.Type.NewStatus + is Changes.LastStatus -> ResultChangeModel.ErrorChange.Type.LastStatus + is Changes.ErrorChanges.Same -> ResultChangeModel.ErrorChange.Type.Same + is Changes.ErrorChanges.Change -> ResultChangeModel.ErrorChange.Type.Change + is Changes.ErrorChanges.Failed -> ResultChangeModel.ErrorChange.Type.Failed + is Changes.ErrorChanges.Fixed -> ResultChangeModel.ErrorChange.Type.Fixed } private fun map(value: StatusChange): ResultChangeModel.StatusChange = @@ -57,18 +64,18 @@ class ResultChangeModelMapper { DescriptionChange.Missing -> "" } - private val Changes.previous: Result? - get() = - when (this) { - is Changes.Status -> previous - is Changes.NewStatus -> null - is Changes.LastStatus -> previous - is Changes.None -> null - is Changes.ErrorChanges.Change -> oldError - is Changes.ErrorChanges.Failed -> null - is Changes.ErrorChanges.Fixed -> oldError - is Changes.ErrorChanges.Same -> error - } + private fun map(statuses: List, changes: Map): List = + statuses.map { lineStatus -> + LineStatusModel( + line = lineStatus.line, + type = lineStatus.type, + description = lineStatus.description, + changeStatus = changes[lineStatus.line]?.let(::map), + changeDescription = (changes[lineStatus.line] as? HasDescriptionChange)?.let { diffDesc(it.desc) }, + active = lineStatus.isActive, + branchDescription = describe(lineStatus.branchStatuses), + ) + } private val Changes.current: Result? get() = @@ -76,7 +83,7 @@ class ResultChangeModelMapper { is Changes.Status -> current is Changes.NewStatus -> current is Changes.LastStatus -> null - is Changes.None -> null + is Changes.Inconclusive -> null is Changes.ErrorChanges.Change -> newError is Changes.ErrorChanges.Failed -> newError is Changes.ErrorChanges.Fixed -> newResult diff --git a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangesCalculator.kt b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangesCalculator.kt index 875879e2..251f6ae2 100644 --- a/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangesCalculator.kt +++ b/web/status-history/src/main/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangesCalculator.kt @@ -24,7 +24,7 @@ class ResultChangesCalculator { oldResult == null && newResult != null -> Changes.NewStatus(newResult) oldResult != null && newResult == null -> Changes.LastStatus(oldResult) oldResult != null && newResult != null -> diffResults(oldResult, newResult) - else /* oldResult == null && newResult == null */ -> Changes.None + else /* oldResult == null && newResult == null */ -> Changes.Inconclusive } private fun diffResults(oldResult: Result, newResult: Result): Changes = diff --git a/web/status-history/src/main/resources/views/LineStatus.hbs b/web/status-history/src/main/resources/views/LineStatus.hbs index 1038b9d0..27677988 100644 --- a/web/status-history/src/main/resources/views/LineStatus.hbs +++ b/web/status-history/src/main/resources/views/LineStatus.hbs @@ -71,51 +71,50 @@ {{#each feedChanges as | feedChange | }} - {{assign "feed" feedChange.current}} + {{#if (empty feedChange.error.header)}} + {{/if}} - {{#each feed.content.lineStatuses as | lineStatus | }} - {{assign "changeStatus" (lookupMap feedChange.statuses lineStatus.line)}} - {{assign "changeDescription" (lookupMap feedChange.descriptions lineStatus.line)}} + {{#each feedChange.statuses as | status | }} {{assign "delayStyle" ""}} - {{#if (not (empty lineStatus.description))}} + {{#if (not (empty status.description))}} {{assign "delayStyle" (concat delayStyle " hasDetails")}} {{/if}} - {{#if (not lineStatus.active)}} + {{#if (not status.active)}} {{assign "delayStyle" (concat delayStyle " inactive")}} {{/if}} - - + - @@ -124,13 +123,13 @@
{{add @index 1}}/{{../feedChanges.length}}: - {{formatDate feed.when "yyyy-MM-dd HH:mm:ss"}} + {{formatDate feedChange.when "yyyy-MM-dd HH:mm:ss"}}
Line Disruption Type Change
{{lineStatus.line.title}} - {{#if (or (empty lineStatus.description) (empty lineStatus.branchDescription))}} - {{lineStatus.type.title}} + {{status.line.title}} + {{#if (or (empty status.description) (empty status.branchDescription))}} + {{status.type.title}} {{else}} - {{lineStatus.type.title}} + {{status.type.title}}
-

{{lineStatus.description}}

-

{{lineStatus.branchDescription}}

+

{{status.description}}

+

{{status.branchDescription}}

{{/if}} 		- {{#if (empty changeDescription)}} - {{changeStatus.title}} + + {{#if (empty status.changeDescription)}} + {{status.changeStatus.title}} {{else}} - {{changeStatus.title}} -
{{{changeDescription}}}
+ {{status.changeStatus.title}} +
{{{status.changeDescription}}}
{{/if}}
- {{#if (not (empty feed.errorHeader))}} - {{feedChange.error.title}}: {{feed.errorHeader}} + {{#if (not (empty feedChange.error.header))}} + {{feedChange.error.type.title}}: {{feedChange.error.header}}
- 
{{feed.fullError}}
+ 
{{feedChange.error.full}}
- {{else if (not (empty feedChange.error.title))}} - {{feedChange.error.title}} + {{else if (not (empty feedChange.error.type.title))}} + {{feedChange.error.type.title}} {{else}} {{!-- Make sure there's always content, otherwise the height of the tables is not the same --}}   diff --git a/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeCalculatorUnitTest_NonContentDiffs.kt b/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeCalculatorUnitTest_NonContentDiffs.kt index 8d0645a0..2fd2231c 100644 --- a/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeCalculatorUnitTest_NonContentDiffs.kt +++ b/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeCalculatorUnitTest_NonContentDiffs.kt @@ -91,7 +91,7 @@ class ResultChangeCalculatorUnitTest_NonContentDiffs { @Test fun testMissingResults() { val difference = subject.diff(null, null) - assertEquals(difference, Changes.None) + assertEquals(difference, Changes.Inconclusive) } } diff --git a/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelUnitTest.kt b/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelUnitTest.kt deleted file mode 100644 index 3d912bfe..00000000 --- a/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultChangeModelUnitTest.kt +++ /dev/null @@ -1,26 +0,0 @@ -package net.twisterrob.travel.statushistory.viewmodel - -import net.twisterrob.travel.statushistory.viewmodel.ResultChangeModel.ErrorChange -import net.twisterrob.travel.statushistory.viewmodel.ResultChangeModel.StatusChange -import org.hamcrest.MatcherAssert.assertThat -import org.hamcrest.Matchers.emptyOrNullString -import org.hamcrest.Matchers.`is` -import org.hamcrest.Matchers.not -import org.hamcrest.Matchers.notNullValue -import org.junit.jupiter.api.Test - -class ResultChangeModelUnitTest { - - @Test fun testStatusChange() { - for (change in StatusChange.entries) { - assertThat(change.name, change.title, `is`(notNullValue())) - assertThat(change.name, change.cssClass, not(`is`(emptyOrNullString()))) - } - } - - @Test fun testErrorChange() { - for (change in ErrorChange.entries) { - assertThat(change.name, change.title, `is`(notNullValue())) - } - } -} diff --git a/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultErrorResultErrorUnitTest.kt b/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultErrorResultErrorUnitTest.kt index bcef9318..10407747 100644 --- a/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultErrorResultErrorUnitTest.kt +++ b/web/status-history/src/test/java/net/twisterrob/travel/statushistory/viewmodel/ResultErrorResultErrorUnitTest.kt @@ -11,7 +11,7 @@ class ResultErrorResultErrorUnitTest { val result = Result.ErrorResult.Error(error) - assertEquals(error, result.error) + assertEquals(error, result.text) assertEquals(errorHeader, result.header) } @@ -21,7 +21,7 @@ class ResultErrorResultErrorUnitTest { val result = Result.ErrorResult.Error(error) - assertEquals(error, result.error) + assertEquals(error, result.text) assertEquals(errorHeader, result.header) } @@ -31,7 +31,7 @@ class ResultErrorResultErrorUnitTest { val result = Result.ErrorResult.Error(error) - assertEquals(error, result.error) + assertEquals(error, result.text) assertEquals(errorHeader, result.header) } }