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

Vouchers types #33

Merged
merged 9 commits into from
Feb 10, 2021
Merged

Vouchers types #33

merged 9 commits into from
Feb 10, 2021

Conversation

pkgacek
Copy link
Contributor

@pkgacek pkgacek commented Jan 15, 2021

No description provided.

@pkgacek
Copy link
Contributor Author

pkgacek commented Jan 29, 2021

Vouchers updated - as talked about with @darekg11

Copy link
Contributor

@darekg11 darekg11 left a comment

Choose a reason for hiding this comment

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

Request changes:

  • type definitions

packages/sdk/src/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
@pkgacek
Copy link
Contributor Author

pkgacek commented Feb 1, 2021

Hey, I've made requested changes.

Copy link
Contributor

@darekg11 darekg11 left a comment

Choose a reason for hiding this comment

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

Requsted changes:

  • Error in list vouchers GET request and possible other GET requests that rely on nested params structure
  • Missing types
  • Missing API methods

packages/sdk/src/types/Vouchers.ts Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Outdated Show resolved Hide resolved
}
/**
* @see https://docs.voucherify.io/reference?utm_source=github&utm_medium=sdk&utm_campaign=acq#list-vouchers
*/
public list(params?: $FixMe) {
return this.client.get('/vouchers', params)
public list(params: T.VouchersListParams = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is broken, try following code:

voucherify.vouchers
	.list({
		created_at: {
			after: '2021-01-28T08:00:00Z',
		},
	})
	.then(voucher => {
		console.log('OKS: %j', voucher)
	})
	.catch(error => {
		console.log(error)
	})

It should list vouchers created only after given date, but it is returning everything. I think this is due to the fact that axios (library used for HTTP Request) is automatically escaping params and it ends up as: "created_at": "{\"after\":\"2021-01-28T08:00:00Z\"}" on server-side.

Please check Axios documentation and try to fix it + check every other place in SDK to see if this is also the case somewhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, the seralization was off.

I had to install Qs library for this to work. I've added paramsSerialize to GET method of RequestController, so it will work with other list methods.

Tommorow morning I will check it with other list methods + I will check if those new methods (bulkUpdate, bulkUpdateMetadata) are working correctly.

packages/sdk/src/types/Vouchers.ts Show resolved Hide resolved
packages/sdk/src/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/Vouchers.ts Outdated Show resolved Hide resolved
packages/sdk/src/Vouchers.ts Show resolved Hide resolved
packages/sdk/src/types/Vouchers.ts Show resolved Hide resolved
@pkgacek
Copy link
Contributor Author

pkgacek commented Feb 10, 2021

Changes pushed. I want to wait with testing this seralization - i would like to merge other types and then make those tests (because right now i would need to install this additional Qs package for every branch)

Copy link
Contributor

@darekg11 darekg11 left a comment

Choose a reason for hiding this comment

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

Nice work 👍
Merging.

@darekg11 darekg11 merged commit e90cdec into voucherifyio:main Feb 10, 2021
@github-actions github-actions bot mentioned this pull request Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants