HAI-3321 Complete return values of kaivuilmoitus taydennys #919

umeetiusbaar · 2025-01-10T14:31:31Z

Description

Use checkChangeInYhteystieto for KaivuilmoitusData contacts
New implementation of checkChangesInAreas - it will check changes inside areas, not just do equals comparison
Implementations of Hakemusalue.listChanges: equals check for other than KaivuilmoitusAlue and field-by-field comparison for KaivuilmoitusAlue
Implementation of Tyoalue.listChanges
Implementation of checking changes in haittojenhallintasuunnitelma
Tests for all

Jira Issue: https://helsinkisolutionoffice.atlassian.net/browse/HAI-3321

Type of change

Bug fix
New feature
Other

Instructions for testing

Create a kaivuilmoitus and a täydennyspyyntö for it
Create täydennys, do changes especially in work areas, haittojenhallintasuunnitelma and contacts
Check the returned muutokset value when the form is saved

Checklist:

I have written new tests (if applicable)
I have ran the tests myself (if applicable)
I have made necessary changes to the documentation, link to confluence
or other location:

Other relevant info

Please describe here if there is e.g. some requirements for this change or
other info that the tester/user needs to know.

corvidian · 2025-01-15T14:43:57Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemusalue.kt

+    second: Tyoalue,
+): String? =
+    if (property.get(this) != property.get(second)) {
+        "areas[$alueIndex].tyoalueet[$tyoalueIndex].${property.name}"


I think this method should just return the name of the property. The caller should handle placing the change in the proper context.

I refactored to use a single fun <T : Any> T.checkChange(property: KProperty1<T, Any?>, second: T): String? (in Utils.kt)

corvidian · 2025-01-15T14:47:44Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemusalue.kt

+        "areas[$index].${property.name}"
+    } else null
+
+private fun Tyoalue.checkChange(


The same checkChange method that's used in Hakemus could be used for all of these checkChange methods, if the constraints on the D type were relaxed. The method would also need to be moved to a shared location.

I refactored to use a single fun <T : Any> T.checkChange(property: KProperty1<T, Any?>, second: T): String? (in Utils.kt)

corvidian · 2025-01-15T14:50:50Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemusalue.kt

+        val changes = mutableListOf<String>()
+        checkChange(alueIndex, tyoalueIndex, Tyoalue::geometry, other)?.let { changes.add(it) }
+        return if (changes.isNotEmpty())
+            listOf("areas[$alueIndex].tyoalueet[$tyoalueIndex]") + changes


Instead of taking the previous indexes as parameters, it would be cleaner to take the previous path. So this method doesn't need to assume the context in which it's run. Similar to how the path parameters are used in validations.

This would then reduce to changes + path.

Changed this as suggested.

corvidian · 2025-01-15T15:00:34Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemusalue.kt

+): List<String> {
+    val changedElementsInFirst =
+        firstAreas.withIndex().mapNotNull { (i, tyoalue) ->
+            tyoalue.listChanges(index, i, secondAreas.getOrNull(i))


Since there's only one field that can be different in a tyoalue, it could be checked right here.

I moved the check for Tyoalue.geometry here.

corvidian · 2025-01-15T15:06:00Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemusalue.kt

+    Haittojenhallintatyyppi.entries.mapNotNull { tyyppi ->
+        if (first[tyyppi] != second[tyyppi]) {
+            "areas[$index].haittojenhallintasuunnitelma[${tyyppi.name}]"
+        } else null
+    }


I think this would be clearer if you first filtered out the changed ones and then changed the name.

Suggested change

Haittojenhallintatyyppi.entries.mapNotNull { tyyppi ->

if (first[tyyppi] != second[tyyppi]) {

"areas[$index].haittojenhallintasuunnitelma[${tyyppi.name}]"

} else null

}

Haittojenhallintatyyppi.entries

.filter { tyyppi -> first[tyyppi] != second[tyyppi] }

.map { "areas[$index].haittojenhallintasuunnitelma[${tyyppi.name}]" }

Changed as suggested.

corvidian · 2025-01-15T15:31:19Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemusalue.kt

+                other.haittojenhallintasuunnitelma,
+            )
+        )
+        return if (changes.isNotEmpty()) changes else null


Isn't an empty list a good way to say there were no changes?

Yes, it is. Changed this.

corvidian · 2025-01-15T15:35:59Z

services/hanke-service/src/main/kotlin/fi/hel/haitaton/hanke/hakemus/Hakemus.kt

-            .map { it.index }
-    val elementsInSecondButNotFirst = secondAreas.indices.drop(firstAreas.size)
-    return (changedElementsInFirst + elementsInSecondButNotFirst).map { "areas[$it]" }
+        firstAreas.withIndex().mapNotNull { (i, area) ->


If area.listChanges() would return an empty list when there are no changes this could use flatMap with no reason to flatten separately.

Changed it as suggested.

corvidian · 2025-01-15T15:41:46Z

services/hanke-service/src/test/kotlin/fi/hel/haitaton/hanke/hakemus/HakemusDataTest.kt

+                assertThat(result).containsExactly("name", "startTime")
+            }
+
+            private fun commonFieldCases(): List<Arguments> =


Is there a reason to repeat these tests? Both the code and the data interfaces are shared between the two types of applications.

Removed repeated tests that test the same data

corvidian · 2025-01-15T15:43:40Z

services/hanke-service/src/test/kotlin/fi/hel/haitaton/hanke/hakemus/HakemusDataTest.kt

-            fun `returns several changes when an area is added to the middle`() {
-                val updated =
+            @Nested
+            inner class AreaChanges {


Since the code checking the area changes are in a different class now, maybe the test code should be for a different class as well?

Created HakemusalueTest and placed tests that test a single Hakemusalue there. Kept those tests that test the area collection (changed the inner test class name to AreasChanges).

corvidian · 2025-01-15T15:47:54Z

services/hanke-service/src/test/kotlin/fi/hel/haitaton/hanke/hakemus/HakemusDataTest.kt

+                        val updated =
+                            base.copy(
+                                areas =
+                                    listOf(
+                                        ApplicationFactory.createExcavationNotificationArea(
+                                            tyoalueet =
+                                                listOf(
+                                                    ApplicationFactory.createTyoalue(
+                                                        geometry = GeometriaFactory.polygon()
+                                                    ),
+                                                    ApplicationFactory.createTyoalue(
+                                                        geometry = GeometriaFactory.thirdPolygon()
+                                                    ),
+                                                    ApplicationFactory.createTyoalue(
+                                                        geometry = GeometriaFactory.thirdPolygon()
+                                                    ),
+                                                    ApplicationFactory.createTyoalue(
+                                                        geometry = GeometriaFactory.secondPolygon()
+                                                    ),
+                                                )
+                                        )
+                                    )
+                            )


The deep nesting makes these really hard to read. Some intermediary values would make these a lot more readable.

Suggested change

val updated =

base.copy(

areas =

listOf(

ApplicationFactory.createExcavationNotificationArea(

tyoalueet =

listOf(

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.polygon()

),

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.thirdPolygon()

),

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.thirdPolygon()

),

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.secondPolygon()

),

)

)

)

)

val tyoalueet =

listOf(

ApplicationFactory.createTyoalue(geometry = GeometriaFactory.polygon()),

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.thirdPolygon()

),

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.thirdPolygon()

),

ApplicationFactory.createTyoalue(

geometry = GeometriaFactory.secondPolygon()

),

)

val area =

ApplicationFactory.createExcavationNotificationArea(tyoalueet = tyoalueet)

val updated = base.copy(areas = listOf(area))

Changed as suggested.

…hanges in contacts and in areas and work areas.

…ludes changes in contacts and in areas and work areas.

…his includes changes in contacts and in areas and work areas.

sonarqubecloud · 2025-01-16T16:03:49Z

Quality Gate failed

Failed conditions
9.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

corvidian reviewed Jan 15, 2025

View reviewed changes

umeetiusbaar added 3 commits January 16, 2025 14:25

HAI-3321 Implement listChanges for KaivuilmoitusData. This includes c…

13bebfa

…hanges in contacts and in areas and work areas.

fixup! HAI-3321 Implement listChanges for KaivuilmoitusData. This inc…

c704136

…ludes changes in contacts and in areas and work areas.

fixup! fixup! HAI-3321 Implement listChanges for KaivuilmoitusData. T…

f4c4a9d

…his includes changes in contacts and in areas and work areas.

umeetiusbaar force-pushed the HAI-3321/complete-return-values-of-kaivuilmoitus-taydennys branch from c02dd84 to f4c4a9d Compare January 16, 2025 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HAI-3321 Complete return values of kaivuilmoitus taydennys #919

HAI-3321 Complete return values of kaivuilmoitus taydennys #919

umeetiusbaar commented Jan 10, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025 •

edited

Loading

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

corvidian Jan 15, 2025

umeetiusbaar Jan 16, 2025

sonarqubecloud bot commented Jan 16, 2025

HAI-3321 Complete return values of kaivuilmoitus taydennys #919

Are you sure you want to change the base?

HAI-3321 Complete return values of kaivuilmoitus taydennys #919

Conversation

umeetiusbaar commented Jan 10, 2025

Description

Jira Issue: https://helsinkisolutionoffice.atlassian.net/browse/HAI-3321

Type of change

Instructions for testing

Checklist:

Other relevant info

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corvidian Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 16, 2025

Quality Gate failed

corvidian Jan 15, 2025 •

edited

Loading