-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(api): api docs not generated and added more e2e tests #2665
fix(api): api docs not generated and added more e2e tests #2665
Conversation
NV-1620 🐛 Bug Report: Update subscriber preferences expected payload and validation mismatch
📜 DescriptionSome of our users have reported that the endpoint The endpoint controller logic (
But the validation applied is reused from the Widget module DTO
There the type assigned to channel is ChannelPreference .
Which is expected to be a nested object in the shape of:
👟 Reproduction stepsIn any environment to do a cURL call to the API.
This will raise an error:
👍 Expected behaviorUpdate the controller to satisfy the validation and follow the same structure as in the Widget module. 👎 Actual Behavior with Screenshots-- 📃 Provide any additional context for the Bug.Test suite for update subscriber preferences is applying a different payload (the one used in Widget module). This means the tests are not accurate. Also we are not checking if the values are updated properly. 👀 Have you spent some time to check if this bug has been raised before?
🏢 Have you read the Contributing Guidelines?
Are you willing to submit PR?Yes I am willing to submit a PR! |
expect(createdSubscriber?.firstName).to.equal('John'); | ||
expect(createdSubscriber?.email).to.equal('john@doe.com'); | ||
expect(createdSubscriber?.phone).to.equal('+972523333333'); | ||
expect(createdSubscriber?.locale).to.equal('en'); |
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.
Linter warnings fixes
expect(createdSubscriber?.firstName).to.equal('Mary'); | ||
expect(createdSubscriber?.email).to.equal('john@doe.com'); | ||
expect(createdSubscriber?.locale).to.equal('en'); |
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.
Linter warnings fixes
👇 Click on the image for a new way to code review
Legend |
@@ -32,7 +32,7 @@ describe('Delete Subscriber - /subscribers/:subscriberId (DELETE)', function () | |||
); | |||
|
|||
const createdSubscriber = await subscriberRepository.findBySubscriberId(session.environment._id, '123'); | |||
expect(createdSubscriber.subscriberId).to.equal('123'); | |||
expect(createdSubscriber?.subscriberId).to.equal('123'); |
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.
Linter warnings fixes
expect(createdSubscriber?.firstName).to.equal('John'); | ||
expect(createdSubscriber?.lastName).to.equal('Test Changed'); | ||
expect(createdSubscriber?.email).to.equal('changed@mail.com'); | ||
expect(createdSubscriber?.phone).to.equal('+972523333333'); | ||
expect(createdSubscriber?.locale).to.equal('sv'); |
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.
Linter warnings fixes
const subscriberChannel = createdSubscriber?.channels?.find((channel) => channel.providerId === 'slack'); | ||
|
||
expect(subscriberChannel.providerId).to.equal('slack'); | ||
expect(subscriberChannel.credentials.webhookUrl).to.equal('webhookUrlNew'); | ||
expect(subscriberChannel?.providerId).to.equal('slack'); | ||
expect(subscriberChannel?.credentials.webhookUrl).to.equal('webhookUrlNew'); |
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.
Linter warnings fixes
...(typeof body.enabled === 'boolean' && { enabled: body.enabled }), | ||
...(body.channel && { channel: body.channel }), |
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.
Protection against wrong payloads.
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.
is undefined
considered a wrong payload? if yes shouldn't we then modify the UpdateSubscriberPreferenceRequestDto
class?
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.
Is used in other controller endpoints so for now I'd rather not touch it, just in case.
47d87ff
to
aa02ec0
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.
you rock🤘
...(typeof body.enabled === 'boolean' && { enabled: body.enabled }), | ||
...(body.channel && { channel: body.channel }), |
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.
is undefined
considered a wrong payload? if yes shouldn't we then modify the UpdateSubscriberPreferenceRequestDto
class?
* Unless explicitly set to false when creating a user preference we want it to be enabled | ||
* even if not passing at first enabled to true. | ||
*/ | ||
enabled: command.enabled !== false, |
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.
enabled: command.enabled !== false, | |
enabled: command.enabled ?? true, |
06b0bc9
to
cd6e76c
Compare
cd6e76c
to
ebde451
Compare
What change does this PR introduce?
Adds more defensive checks in the code and avoids to accept bad formatted payloads. Adds more E2E tests for the update of the subscriber preferences after some investigation regarding potential bugs reported by users. Also improves the API docs with more detailed descriptions.
Why was this change needed?
Improve the confidence in the code and clarify the intention to the users.
Other information (Screenshots)