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

fix: Update Subscribers API allowing null #3169

Merged
merged 4 commits into from
Jun 16, 2023

Conversation

jayavardhan3112
Copy link
Contributor

@jayavardhan3112 jayavardhan3112 commented Apr 10, 2023

What change does this PR introduce?

Changed the update subscriber API, thus when there is a null or empty string, it will unset the email from the database

Why was this change needed?

Closes #3022

Copy link
Contributor

@scopsy scopsy left a comment

Choose a reason for hiding this comment

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

Because currently the endpoint behaving like a partial update endpoint with all fields this can lead to situation where the email is removed when updating other fields. Can we maybe perform a check like it was previously, but instead of != null we can check for !== undefined so in case null was passed, we will update the field to null instead of unsettling it. wdyt?

@jayavardhan3112
Copy link
Contributor Author

Because currently the endpoint behaving like a partial update endpoint with all fields this can lead to situation where the email is removed when updating other fields. Can we maybe perform a check like it was previously, but instead of != null we can check for !== undefined so in case null was passed, we will update the field to null instead of unsettling it. wdyt?

So when a user passes an empty string on Null, then only the email would be unset, (The reason I unset the Email Key is what if its empty string , or Null, this would make the code to add redundant checks), thus I thought of unsetting the key,

@scopsy
Copy link
Contributor

scopsy commented Apr 13, 2023

@jayavardhan3112 yes exactly, when it's undefined it should be untouched. I think setting it as null can help make queries for those users vs unset (meaning it was never set) similar to the semantic undefined vs null situation. Wdyt?

@jayavardhan3112
Copy link
Contributor Author

@jayavardhan3112 yes exactly, when it's undefined it should be untouched. I think setting it as null can help make queries for those users vs unset (meaning it was never set) similar to the semantic undefined vs null situation. Wdyt?

makes sense, I will update this and let you know, Thnx

@jayavardhan3112
Copy link
Contributor Author

Updated, Please Review

Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

🌟
@jayavardhan3112 please solve the conflicts so we can merge it. Sorry it took us so long. 🙏🏻

@jayavardhan3112
Copy link
Contributor Author

Updated, @p-fernandez . am not able to resolve them, could you please check the access

@p-fernandez
Copy link
Contributor

p-fernandez commented Jun 16, 2023

@jayavardhan3112 could you give me write permissions in your repository to push the conflict fix in your next branch? The PR was closed because I could only push Novu's next branch so the PR gets invalidated.
By the way don't worry as I have your PR fixed in a branch in my repository.

@jayavardhan3112
Copy link
Contributor Author

@jayavardhan3112 could you give me write permissions in your repository to push the conflict fix in your next branch? The PR was closed because I could only push Novu's next branch so the PR gets invalidated.
By the way don't worry as I have your PR fixed in a branch in my repository.

Hi @p-fernandez , Have invited you to collaborate to the PR

@github-actions github-actions bot removed the @novu/ws label Jun 16, 2023
@p-fernandez
Copy link
Contributor

@jayavardhan3112 done. Can you do a quick review and confirm all your changes are there?

@jayavardhan3112
Copy link
Contributor Author

@jayavardhan3112 done. Can you do a quick review and confirm all your changes are there?

Looks good to me, you can go ahead and merge it, Thank you

@p-fernandez p-fernandez added this pull request to the merge queue Jun 16, 2023
Merged via the queue into novuhq:next with commit a17b1e6 Jun 16, 2023
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NV-1847] 🐛 Bug Report: Update API accepts null or '' as values but email is not unset
4 participants