Convert transferSlack configuration to duration

[leonardehrenfried]

Summary

Convert the transferSlack configuration option to a duration. In order to preserve backwards-compatibility with Entur's stack, this is backwards compatible.

Unit tests

Added for config value and API mappers.

Documentation

Updated.

[codecov]

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.45%. Comparing base (2a5c8be) to head (468bb83).

Files	Patch %	Lines
.../restapi/resources/RequestToPreferencesMapper.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5897      +/-   ##
=============================================
- Coverage      69.45%   69.45%   -0.01%     
+ Complexity     17070    17068       -2     
=============================================
  Files           1937     1937              
  Lines          73692    73699       +7     
  Branches        7540     7542       +2     
=============================================
  Hits           51184    51184              
- Misses         19880    19884       +4     
- Partials        2628     2631       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[optionsome]

alightSlackForMode*, for some reason there isn't suggestion option when adding review comment 🤔 .

[optionsome]

Ah I think it was because I was reviewing commit instead of overall changes.

[optionsome]

I wasn't properly concentrating today during the developer meeting discussion, but isn't this the description of transferSlack, not alightSlack? I thought alightSlack was mostly meant just for random procedures you have to do after you alight from a vehicle like with airplanes. Although, this is the generic one and I'm not even completely sure what this is for. Maybe if you are traveling on wheelchair this could be useful you if you need assistance or if the vehicles are crowded and getting off a vehicle takes time.

Referring to connection feels a bit weird. This feels more like a safety margin that you get to the destination in time.

[leonardehrenfried]

I agree with you a wrote something to that effect previously but @abyrd and @t2gran dictated this to me word for word.

I'm feeling that I should just revert the change to the documentation and we pick that up later.

[abyrd]

It's possible that I misinterpreted the purpose of the different parameters. I tried to verify with everyone that any strong statements were accurate. I'm happy to rewrite it if it's incorrect.

[t2gran]

To me the description is good. The following sentence could be better:

The sole reason for this is to avoid missed connections when there are minor schedule variations.

It should emphasise "minor schedule variations". Note! It is probably a better simulation of real life to have 20s boardSlack and 10s alightSlack, than 30s/0s. But, due to technical difficulties in presenting this to the end user, most deployments uses just boardSlack. I do not recommend using this for user-specific needs like wheelchair.

We can refrase to, if less confusing:

The main reason is to account for minor schedule variations.

[optionsome]

Again, connection to me refers more to a transfer than potentially the only boarding but I'm not a native speaker.

[leonardehrenfried]

I agree, however please see above.

[abyrd]

Here are some suggested revisions reflecting @joelhaasnoot's comments and @leonardehrenfried's clarifications.

[t2gran]

This need a comment or a different name. I assume it is testing backward compatibility of the old name?

If you try to do two things at once, I would instead make two tests instead. Then the intention can be communicated in the test name.

[leonardehrenfried]

It is indeed for backwards-compatibility: I clarifi€d in 468bb83

[t2gran]

I would have written this like the suggestion below witch I think is fare more readable. The reason why this test is confusing is that the variable arguments is not used to provide different test-cases, but alias/old name of the property. Then, if you need to extend the test-cases here it become very ugly. It also test two different things - we are not strict about that, but to some it might be confusing. You could also split it in 3 methods => no need for any comment, just clear names.

@Test
public void testTransferSlack() {
    var d101s = Duration.ofSeconds(101);
    var req = TripRequestMapper.createRequest(executionContext(Map.of("transferSlack", 101)));
    assertEquals(d101s, req.preferences().transfer().slack());

    assertThrows(..., () ->
      TripRequestMapper.createRequest(executionContext(Map.of("transferSlack", <invalid value>)));
    );
    
    // Test the deprecated  'minimumTransferTime' for backwards compatibility
    req = TripRequestMapper.createRequest(executionContext(Map.of("transferSlack", 101)));
    assertEquals(d101s, req.preferences().transfer().slack());
}