Add GET API for fetching combine target frame

[Leitsi]

This change is 

[jarkkoka]

Reviewed 30 of 30 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/TimetablesController.kt line 134 at r1 (raw file):

        val targetPriorityEnumResult = runCatching { TimetablesPriority.fromInt(targetPriority) }
        if (targetPriorityEnumResult.isFailure) {

This snippet of code does not look like idiomatic Kotlin.

I really would like to change the implementation of TimetablesPriority.fromInt into this form:

fun fromInt(value: Int): TimetablesPriority? = values().firstOrNull { it.value == value }

Then, you could implement this in a much more elegant way:

val targetPriorityEnum =  
    TimetablesPriority.fromInt(targetPriority)  
        ?: throw TargetPriorityParsingException("Failed to parse target priority", targetPriority)  
  
val targetVehicleScheduleFrame = combineTimetablesService.fetchTargetVehicleScheduleFrame(  
    stagingVehicleScheduleFrameId,  
    targetPriorityEnum  
)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorExtensions.kt line 19 at r1 (raw file):

    // Not required by Hasura, but we want to always pass one.
    val type: HasuraErrorType

See my comments related to this in another PR.

---

src/main/kotlin/fi/hsl/jore4/timetables/api/util/HasuraErrorType.kt line 3 at r1 (raw file):

package fi.hsl.jore4.timetables.api.util

enum class HasuraErrorType {

See my comments related to the name of this enum class in another PR.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesToCombineApiTest.kt line 41 at r1 (raw file):

    )
    private val defaultToCombineTargetId = defaultTargetFrame.vehicleScheduleFrameId
    private fun executeToCombineTimetablesRequest(

Please add line break before a function definition.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesToCombineApiTest.kt line 61 at r1 (raw file):

            combineTimetablesService.fetchTargetVehicleScheduleFrame(
                stagingVehicleScheduleFrameId,
                TimetablesPriority.fromInt(targetPriority)

Be explicit here:

TimetablesPriority.TEMPORARY

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesToCombineApiTest.kt line 65 at r1 (raw file):

        } answers { defaultTargetFrame }

        executeToCombineTimetablesRequest(stagingVehicleScheduleFrameId, targetPriority)

You could add this line to the beginning of the method:

val targetPriority = TimetablesPriority.TEMPORARY

Then you reference the int value here by:

targetPriority.value

It really is unnecessary to test parsing done in TimetablesPriority.fromInt here.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesToCombineApiTest.kt line 82 at r1 (raw file):

            combineTimetablesService.fetchTargetVehicleScheduleFrame(
                stagingVehicleScheduleFrameId,
                TimetablesPriority.fromInt(targetPriority)

Be explicit here:

TimetablesPriority.TEMPORARY

---

src/test/kotlin/fi/hsl/jore4/timetables/api/util/TransactionSystemExtensionsTest.kt line 10 at r1 (raw file):

    private fun createTransactionSystemExceptionWithCause(message: String): TransactionSystemException {
        return TransactionSystemException("test exception", Exception(message))

The method body could be simplified to expression form:

private fun createTransactionSystemExceptionWithCause(message: String) =  
    TransactionSystemException("test exception", Exception(message))

No need to duplicate the return type.

---

src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt line 366 at r1 (raw file):

            val causeMessage = exception?.cause?.message
            assertNotNull(causeMessage)
            assertContains(causeMessage, "ERROR: conflicting schedules detected: vehicle schedule frame")

My current favourite to write this is:

assertFailsWith<TransactionSystemException> {  
    combineTimetablesService.combineTimetables(  
        listOf(stagingFrameId),  
        TimetablesPriority.STANDARD  
    )  
}.also { exception ->  
    // Check that the error messages are somewhat in the format we expect. TransactionSystemExtensions depends on these.  
    assertEquals(exception.message, "JDBC commit failed")  
  
    val causeMessage = exception.cause?.message  
    assertNotNull(causeMessage)  
    assertContains(causeMessage, "ERROR: conflicting schedules detected: vehicle schedule frame")  
    assertContains(causeMessage, "Where: PL/pgSQL function vehicle_schedule.validate_queued_schedules_uniqueness()")  
    assertContains(causeMessage, "SQL statement \"SELECT vehicle_schedule.validate_queued_schedules_uniqueness()")  
}

[Leitsi]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jarkkoka)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/TimetablesController.kt line 134 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

This snippet of code does not look like idiomatic Kotlin.

I really would like to change the implementation of TimetablesPriority.fromInt into this form:

fun fromInt(value: Int): TimetablesPriority? = values().firstOrNull { it.value == value }

Then, you could implement this in a much more elegant way:

val targetPriorityEnum =  
    TimetablesPriority.fromInt(targetPriority)  
        ?: throw TargetPriorityParsingException("Failed to parse target priority", targetPriority)  
  
val targetVehicleScheduleFrame = combineTimetablesService.fetchTargetVehicleScheduleFrame(  
    stagingVehicleScheduleFrameId,  
    targetPriorityEnum  
)

Done.

---

src/test/kotlin/fi/hsl/jore4/timetables/api/TimetablesToCombineApiTest.kt line 65 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

You could add this line to the beginning of the method:

val targetPriority = TimetablesPriority.TEMPORARY

Then you reference the int value here by:

targetPriority.value

It really is unnecessary to test parsing done in TimetablesPriority.fromInt here.

Done.

---

src/test/kotlin/fi/hsl/jore4/timetables/service/CombineTimetablesServiceTest.kt line 366 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

My current favourite to write this is:

assertFailsWith<TransactionSystemException> {  
    combineTimetablesService.combineTimetables(  
        listOf(stagingFrameId),  
        TimetablesPriority.STANDARD  
    )  
}.also { exception ->  
    // Check that the error messages are somewhat in the format we expect. TransactionSystemExtensions depends on these.  
    assertEquals(exception.message, "JDBC commit failed")  
  
    val causeMessage = exception.cause?.message  
    assertNotNull(causeMessage)  
    assertContains(causeMessage, "ERROR: conflicting schedules detected: vehicle schedule frame")  
    assertContains(causeMessage, "Where: PL/pgSQL function vehicle_schedule.validate_queued_schedules_uniqueness()")  
    assertContains(causeMessage, "SQL statement \"SELECT vehicle_schedule.validate_queued_schedules_uniqueness()")  
}

Done.

[jarkkoka]

Reviewed 27 of 27 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Leitsi)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/TimetablesController.kt line 55 at r2 (raw file):

    )

    private fun parseTargetPriority(targetPriority: Int) = TimetablesPriority.fromInt(targetPriority)

Since this method is essentially stateless (static) it should be located in a companion object.

[Leitsi]

Reviewable status: 30 of 31 files reviewed, 1 unresolved discussion (waiting on @jarkkoka)

---

src/main/kotlin/fi/hsl/jore4/timetables/api/TimetablesController.kt line 55 at r2 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

Since this method is essentially stateless (static) it should be located in a companion object.

Done.

[jarkkoka]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Leitsi)