-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Db tickets without ticket url #310
Db tickets without ticket url #310
Conversation
e79f9e5
to
f6a7e09
Compare
p/db/index.js
Outdated
jnyCl: opt.firstClass === true ? 1 : 2, | ||
// todo [breaking]: support multiple travelers | ||
tvlrProf: [{ | ||
type: tvlrAgeGroup || ageGroup.ADULT, | ||
...(('age' in opt) ? {age: opt.age} : {}), | ||
redtnCard: opt.loyaltyCard | ||
? formatLoyaltyCard(opt.loyaltyCard) | ||
? (typeof opt.loyaltyCard === 'number' && Number.isInteger(opt.loyaltyCard) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change enable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change allows also the usage of already formattedLoyalty cards like the following:
data = await client.refreshJourney(journey.refreshToken, { stopovers: true, remarks: true, tickets: true, age: 42, loyaltyCard: 4, })
Without it the loyaltyCard would have no effect and would be mapped to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this change and added reverseFormatLoyaltyCard instead.
I guess this is a cleaner approach.
Usage would look like:
import {reverseFormatLoyaltyCard} from './loyalty-cards.js'
...
data = await client.refreshJourney(journey.refreshToken, {
stopovers: true,
remarks: true,
tickets: true,
generateUnreliableTicketUrls: true,
age: 42,
loyaltyCard: reverseFormatLoyaltyCard(4),
})
or
import {data as loyaltyCards} from './loyalty-cards.js'
...
data = await client.refreshJourney(journey.refreshToken, {
stopovers: true,
remarks: true,
tickets: true,
generateUnreliableTicketUrls: true,
age: 42,
loyaltyCard: {type: loyaltyCards.BAHNCARD, discount: 50},
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious: What's the use case for passing in an already formatted (a.k.a. encoded, already in the proprietary DB-HAFAS-specific representation) loyalty card identifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the hafas-client behind another api. From this I just receive the formatted identifier which I will then forward to the hafas-client.
Feel free to remove this mapping if you think its bloating up the codebase :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having in hafas-client
makes sense, given that it is quite DB- and HAFAS-specific compared to most consuming code. However, I try to have all code in the repo to be used at runtime, tested automatically, or both.
Will add some small assertions.
|
||
const queryParams = new URLSearchParams(); | ||
|
||
// Add individual parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for future investigations: This is closely related to, but not the same as, https://github.com/derhuerst/generate-db-shop-urls/blob/e9f5d4385eb5985d52f244921bb7562efc3d7d64/index.js#L99-L136.
test/e2e/db.js
Outdated
test.equal(typeof fare.name, 'string'); | ||
test.ok(fare.name); | ||
} else { | ||
test.fail('Mandatory field "name" is missing'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case is covered by the test.equal(typeof fare.name, 'string')
assertion above. Add an assertion message above, it will imply that the field is mandatory.
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derhuerst I adjusted the tests
0062ce1
to
2fb2e32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one tiny problem left to fix! (Will do that by myself.)
test/e2e/db.js
Outdated
test.equal(typeof fare.name, 'string', 'Mandatory field "name" is missing or not a string'); | ||
test.ok(fare.name); | ||
|
||
test.equal(typeof fare.priceObj, 'object', 'Mandatory field "priceObj" is missing or not an object'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fare.priceObj
might theoretically be null
. You could use the following:
test.ok(isObj(fare.priceObj), 'Mandatory field "priceObj" is missing or not an object');
I will rebase & squash your commits, and then merge the changes. I might need some days for this. |
Co-Authored-By: Jannis R <[email protected]>
Co-Authored-By: Jannis R <[email protected]>
Co-Authored-By: Jannis R <[email protected]>
Co-Authored-By: Jannis R <[email protected]>
Co-Authored-By: Jannis R <[email protected]>
Co-Authored-By: Jannis R <[email protected]>
5b42626
to
d3bc9d3
Compare
Thanks a lot, also for your patience! Do you want to be mentioned in |
Published as |
This PR enables to get the db fares (Super Sparpreis, Flexpreis...) with refreshJourney(tickets:true).
tickets for journeys() or refreshJourney(tickets:false) look like:
and for refreshJourney(tickets:true):
fixes #296
fixes #311