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

Fixes optional price behavior #91

Closed

Conversation

lightsprint09
Copy link

@lightsprint09 lightsprint09 commented Oct 27, 2018

As described in fptf amount and currency should be required. If amount is not available the whole price object is nil

price: { // optional
	amount: 12.50, // number, required
	currency: 'EUR' // ISO 4217 code, required
}

Copy link
Member

@derhuerst derhuerst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also adapt the tests.

@@ -55,20 +55,18 @@ const createParseJourney = (profile, opt, data) => {
// targetCtx: 'D',
// buttonText: 'To offer selection'
// } ]
res.price = {amount: null, hint: 'No pricing information available.'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please assign null as the default value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added null as default

@derhuerst
Copy link
Member

Technically someone could rely one the behaviour that, if journey.price is truthy, it will contain a price. Because of this, I'm not sure if this is a breaking change.

@lightsprint09
Copy link
Author

Technically someone could rely one the behaviour that, if journey.price is truthy, it will contain a price. Because of this, I'm not sure if this is a breaking change.

Right this is a breaking change...

@derhuerst derhuerst added the breaking breaking change label Oct 30, 2018
@derhuerst derhuerst mentioned this pull request Oct 31, 2018
6 tasks
derhuerst added a commit that referenced this pull request Mar 11, 2019
derhuerst added a commit that referenced this pull request Mar 12, 2019
derhuerst added a commit that referenced this pull request Mar 21, 2019
derhuerst added a commit that referenced this pull request May 20, 2019
derhuerst added a commit that referenced this pull request Aug 16, 2019
@derhuerst derhuerst changed the base branch from master to 5 August 16, 2019 17:29
@derhuerst
Copy link
Member

Sorry, I just noticed that I have added this behaviour in 8677a1c on branch 5 already. Nonetheless, thanks for your contribution!

@derhuerst derhuerst closed this Aug 16, 2019
derhuerst added a commit that referenced this pull request Sep 10, 2019
derhuerst added a commit that referenced this pull request Dec 26, 2019
derhuerst added a commit that referenced this pull request Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking breaking change
Development

Successfully merging this pull request may close these issues.

2 participants