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.

Sign up for GitHub

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

Jump to bottom

[PM-8039] Fastmail: Change website default value to empty string. #9127

Merged
merged 2 commits into from
May 30, 2024
Merged

[PM-8039] Fastmail: Change website default value to empty string. #9127

merged 2 commits into from
May 30, 2024

Conversation

dixls
Copy link
Contributor

@dixls dixls commented May 10, 2024 •
edited

Fastmail API returns and error when passed a null value in forDomain. Empty string is the preferred blank option.

Type of change

- [ X ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

null is not allowed in forDomain in Fastmail's API

@dixls dixls requested a review from a team as a code owner May 10, 2024 13:57
@CLAassistant
Copy link

CLAassistant commented May 10, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-8039

@bitwarden-bot bitwarden-bot changed the title Fastmail: Change website default value to empty string. [PM-8039] Fastmail: Change website default value to empty string. May 10, 2024
@audrey-jensen audrey-jensen self-requested a review May 13, 2024 15:16
audrey-jensen

This comment was marked as outdated.

Sign in to view

@dixls

This comment was marked as outdated.

Sign in to view
@audrey-jensen

This comment was marked as outdated.

Sign in to view
@dixls
Copy link
Contributor Author

dixls commented May 13, 2024

Oh I see, I believe this issue would be fixed with the change here, since the default is now a string, but typescript is not my main thing, so let me know if I'm reading this wrong!

@djsmith85 djsmith85 linked an issue May 15, 2024 that may be closed by this pull request
Fastmail "forwarded email alias" masked email returns an error #9183
Closed
1 task
@audrey-jensen
Copy link
Contributor

audrey-jensen commented May 20, 2024 •
edited

I'd like this in the procedural code because the type definitions are in flux and this is fastmail-specific behavior.

There's two spots for that at present.

@dixls
Change website default value to empty string.
73c0a00 
Fastmail API returns and error when passed a null value in forDomain.
Empty string is the preferred blank option.
@dixls dixls force-pushed the patch-null-fordomain-fastmail branch from 386a171 to 73c0a00 Compare May 21, 2024 18:14
@dixls
Copy link
Contributor Author

dixls commented May 21, 2024

I updated this PR with the change in that other spot, but let me know if it makes more sense to close this one and open a PR against the other branch that's in progress.

@audrey-jensen audrey-jensen merged commit c37006c into bitwarden:main May 30, 2024
35 of 46 checks passed
@audrey-jensen
Copy link
Contributor

Sorry for the delay! For some reason your last push didn't come through my notifier! 😬

This is good to go! Thank you! :D

@audrey-jensen
Copy link
Contributor

audrey-jensen commented May 30, 2024 •
edited

@dixls - Looks like you need to sign the CLA with your fastmail account due to 73c0a00. I know they're both you, but we need our bot to know, too. 🙃

audrey-jensen added a commit that referenced this pull request May 30, 2024
@audrey-jensen
Revert "Change website default value to empty string. (#9127)"
fd0488d 
This reverts commit c37006c.
@audrey-jensen audrey-jensen mentioned this pull request May 30, 2024
Merged
@dixls
Copy link
Contributor Author

dixls commented May 30, 2024

I think I got it now lol, whoops! Thanks so much!

audrey-jensen added a commit that referenced this pull request May 30, 2024
@audrey-jensen
Revert "Change website default value to empty string. (#9127)" (#9437)
d0914ac 
This reverts commit c37006c.
@audrey-jensen
Copy link
Contributor

Thanks!

audrey-jensen added a commit that referenced this pull request May 30, 2024
@audrey-jensen
Revert "Revert "Change website default value to empty string. (#9127)" (
b9e8f58 
#9437)"

This reverts commit d0914ac.
audrey-jensen added a commit that referenced this pull request May 30, 2024
@audrey-jensen
Revert "Revert "Change website default value to empty string. (#9127)" (
0ef5ab7 
#9437)" (#9438)

This reverts commit d0914ac.
@audrey-jensen audrey-jensen mentioned this pull request May 30, 2024
Merged
@djsmith85 djsmith85 mentioned this pull request May 31, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audrey-jensen audrey-jensen audrey-jensen approved these changes

Assignees
No one assigned
Labels
community-pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fastmail "forwarded email alias" masked email returns an error
4 participants
@dixls @CLAassistant @bitwarden-bot @audrey-jensen