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 user_context to merge with existing context #889

Closed
wants to merge 1 commit into from

Conversation

meganemura
Copy link
Contributor

No description provided.

@meganemura
Copy link
Contributor Author

Test is failing due to #890

@st0012
Copy link
Collaborator

st0012 commented Aug 21, 2020

thanks for the PR, but I don't see a clear reason for this change. can you give me an example or related issue report?

@st0012 st0012 added the bug fix label Aug 21, 2020
@st0012 st0012 added this to the 3.0.3 milestone Aug 21, 2020
@st0012
Copy link
Collaborator

st0012 commented Aug 27, 2020

I'm closing this now, but feel free to reopen it if you still think it's needed.

@st0012 st0012 closed this Aug 27, 2020
@st0012 st0012 removed this from the 3.0.3 milestone Aug 27, 2020
@agrobbin
Copy link
Contributor

@st0012 I believe this is still a bug, or at least an inconsistency with the documentation on user_context. It states:

# Bind user context. Merges with existing context (if any).

However, the actual implementation appears to overwrite anything that's already set within user_context. For example:

Raven.user_context(id: current_user.id))
Raven.user_context(email: current_user.email) if current_user.email

This will result in the user_context losing the :id info if current_user.email is truthy.

@st0012
Copy link
Collaborator

st0012 commented Oct 15, 2020

@agrobbin thanks for raising this again with examples 👍 I've added #1064 to address that.

@agrobbin
Copy link
Contributor

No problem @st0012, and thanks for the quick fix!!

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.

3 participants