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(project options): Store feedback:branding options correctly #42022

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

ceorourke
Copy link
Member

Toggling off the user feedback branding option wasn't actually doing anything - when the user refreshed the screen it was back to "on". This PR fixes that by grabbing the option correctly - before it was always falling back to the default value and setting it to True.

@ceorourke
Copy link
Member Author

It looks like this change was made recently in #39575 so cc @cathyteng17 and @markstory

Copy link
Member

@cathteng cathteng left a comment

Choose a reason for hiding this comment

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

thanks for catching this!

@@ -184,7 +184,7 @@ def format_options(attrs: defaultdict(dict)):
f"filters:{FilterTypes.ERROR_MESSAGES}": "\n".join(
attrs["options"].get(f"sentry:{FilterTypes.ERROR_MESSAGES}", [])
),
"feedback:branding": attrs.get("feedback:branding", "1") == "1",
"feedback:branding": attrs["options"].get("feedback:branding", "1") == "1",
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on it, but is "sentry:reprocessing_active": bool(attrs.get("sentry:reprocessing_active", False)), also broken?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should do

options = attrs["options"]
....
    options.get(...)

Might be less error prone

Copy link
Member

Choose a reason for hiding this comment

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

i think you're right (on both counts)

@vercel
Copy link

vercel bot commented Dec 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sentry ✅ Ready (Inspect) Visit Preview Dec 5, 2022 at 10:37PM (UTC)
storybook ✅ Ready (Inspect) Visit Preview Dec 5, 2022 at 10:37PM (UTC)

@ceorourke ceorourke merged commit a9ba7ef into master Dec 5, 2022
@ceorourke ceorourke deleted the ceorourke/user-feedback-branding branch December 5, 2022 23:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants