Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for age-based tariffs in DB profile #264

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

roehrt
Copy link
Contributor

@roehrt roehrt commented Apr 21, 2022

The supported age groups would be:

  • B (baby, infant)
  • K (kind, child)
  • Y (young)
  • E (Erwachsener, adult)
  • S (Senior)

(all taken from a decompiled DB Navigator)

Note: The age groups Y and S are fairly new, so in order for them to work, the version in base.json needed to be updated to a newer one otherwise HAFAS responds with an PARSE error (only ext seems to matter but I updated the others as well).
I don't know if and how the version update affects other methods, but at first glance everything seems to work fine.

Would also resolve a problem mentioned in #235

Intended Usage

const dbProfile = require('hafas-client')
const {ageGroup, ageGroupFromAge} = require('hafas-client/p/db/ageGroup')
const client = createClient(dbProfile, 'hafas-client-example')

const print = (...data) => console.log(require('util').inspect(...data, {depth: null, colors: true}));

// Berlin Jungfernheide to München Hbf
client.journeys('8011167', '8000261', { results: 1, tickets: true, ageGroup: 'E' }).then(print)
client.journeys('8011167', '8000261', { results: 1, tickets: true, ageGroup: ageGroup.YOUNG }).then(print)
client.journeys('8011167', '8000261', { results: 1, tickets: true, ageGroup: ageGroupFromAge(12) }).then(print)

If no ageGroup is passed it defaults to adult to maintain compatibility to prior versions.

@roehrt roehrt changed the title Add support for age-based tariffs for DB profile Add support for age-based tariffs in DB profile Apr 21, 2022
@derhuerst
Copy link
Member

Thanks for this excellent PR!

[…] the version in base.json needed to be updated to a newer one otherwise HAFAS responds with an PARSE error (only ext seems to matter but I updated the others as well). I don't know if and how the version update affects other methods, but at first glance everything seems to work fine.

Would you mind to determine and use the lowest ext possible that supports Y/S? This way, we reduce the likelihood of breakage.

// …
client.journeys('8011167', '8000261', { results: 1, tickets: true, ageGroup: ageGroupFromAge(12) }).then(print)

What do you think about changing the API to age: 12 in this case? It has much better ergonomics IMO, at the cost of introducing another option (that overlaps in functionality with ageGroup).


Would you like to be mentioned as a contributor? If you do, there are several supported formats.

@derhuerst
Copy link
Member

With the current test setup, unfortunately it is expected that some integration tests fail, but given that they store response fixtures given a hash of the request, the db tests fail now because the tvlrProf.type field has changed.

There is another problem though: In order to have unchanging requests (so that the test fixtures can be used), all requests need to be made for a fixed point in time. This point in time currently applies to the integration tests of all profiles, so

  1. in order to adapt the integration test fixtures to match requests with tvlrProf.type, we need to update the fixtures,
  2. which requires saving "real" responses from the DB HAFAS API as fixtures,
  3. which means that the fixed point in time (T_MOCK in test/e2e/lib/util.js) needs to be updated not to be too far in the past.

TLDR: Updating all integration tests' fixtures because a single profile changed its request signature is a major hassle. I'm not happy with this setup, but so far haven't managed to do something about it.

@derhuerst
Copy link
Member

TLDR: Updating all integration tests' fixtures because a single profile changed its request signature is a major hassle. I'm not happy with this setup, but so far haven't managed to do something about it.

In 1ab526d, I have changed the integration tests to individually define their T_MOCK, so that we can update the DB integration tests more easily now.

@roehrt
Copy link
Contributor Author

roehrt commented Apr 23, 2022

Would you mind to determine and use the lowest ext possible that supports Y/S? This way, we reduce the likelihood of breakage.

ext should now be the lowest possible one and I rolled back the ver since it didn't really seem to influence the behavior of HAFAS.

It has much better ergonomics IMO, at the cost of introducing another option (that overlaps in functionality with ageGroup).

It's definitely handier; I'd also say that it fits in hafas-client quite well. I drafted it quickly.

Would you like to be mentioned as a contributor? If you do, there are several supported formats.

Sure, thanks!

@derhuerst
Copy link
Member

Would you like to be mentioned as a contributor? If you do, there are several supported formats.

Sure, thanks!

How would you like to be mentioned? roehrt? Tobias Roehr? Keep in mind that this is permanently readable metadata.

roehrt added 3 commits April 26, 2022 12:13
The supported age groups are now:
- 'B' (baby, infant)
- 'K' (kind, child)
- 'Y' (young)
- 'E' (Erwachsener, adult)
- 'S' (Senior)
@derhuerst
Copy link
Member

FYI: I have done some minor squashing & renaming of your commits, and checked in new integration test fixtures for the DB profile.

@roehrt
Copy link
Contributor Author

roehrt commented Apr 26, 2022

How would you like to be mentioned?

roehrt is fine. Is there anything left I can do regarding testing? (I'm a newbie to the GitHub workflow for testing)

@derhuerst derhuerst merged commit 119d291 into public-transport:master Apr 26, 2022
@derhuerst
Copy link
Member

Nothing left to do!
I have merged your changes and published them as [email protected].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants