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

Resend Verification Email HS #1767

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Resend Verification Email HS #1767

merged 1 commit into from
Jan 7, 2025

Conversation

NolanTrem
Copy link
Collaborator

@NolanTrem NolanTrem commented Jan 7, 2025

Important

Add Content-Type: text/plain header to sendVerificationEmail request in users.ts.

  • Behavior:
    • Modify sendVerificationEmail in users.ts to include Content-Type: text/plain header in the request.

This description was created by Ellipsis for 670f0d8. It will automatically update as commits are pushed.

@NolanTrem NolanTrem merged commit fe54b6e into main Jan 7, 2025
3 checks passed
@NolanTrem NolanTrem deleted the Nolan/JSFix branch January 7, 2025 09:21
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 670f0d8 in 30 seconds

More details
  • Looked at 16 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/sdk/src/v3/clients/users.ts:84
  • Draft comment:
    The data field should be an object, not a plain string. Change data: options.email to data: { email: options.email }.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The change appears intentional - they're explicitly setting Content-Type to text/plain which matches the new data format. Other endpoints in this file use both styles - some with JSON objects, others with plain text. Without seeing the API spec or server code, I can't be certain which format this endpoint expects. The explicit Content-Type header suggests this was a deliberate change to match the API's requirements.
    I could be wrong about the API requirements - maybe it really does need JSON. The presence of similar endpoints using both formats makes it unclear which is correct.
    Without clear evidence that this endpoint requires JSON format, and given the explicit Content-Type header suggesting this was intentional, I should err on the side of assuming the author knows the API requirements.
    Delete the comment. While it points out a real change in the data format, we don't have strong evidence that this change is incorrect, and the explicit Content-Type header suggests it was intentional.

Workflow ID: wflow_HbTBDl406vbnom7M


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant