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

Fail to render event_details when there's more than one UserReport for an event id #3140

Merged
merged 1 commit into from May 9, 2016

Conversation

mattrobenolt
Copy link
Contributor

Fixes SENTRY-16W

@getsentry/infrastructure

@codecov-io
Copy link

codecov-io commented Apr 28, 2016

Current coverage is 82.49%

Merging #3140 into master will increase coverage by +<.01%

  1. File ...y/models/eventtag.py (not in diff) was modified. more
    • Misses 0
    • Partials 0
    • Hits -1
@@             master      #3140   diff @@
==========================================
  Files           941        941          
  Lines         37018      37033    +15   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          30534      30549    +15   
  Misses         6484       6484          
  Partials          0          0          

Powered by Codecov. Last updated by 1c5ae57...204233d

@JTCunning
Copy link
Contributor

LGTM

@mattrobenolt
Copy link
Contributor Author

@ckj brought up a good point. Why were there two UserReports here in the first place. There should only ever be one.

@dcramer
Copy link
Member

dcramer commented Apr 28, 2016

null unique constraint?

@mattrobenolt
Copy link
Contributor Author

null unique constraint?

Why a null? We can do a UNIQUE on (project_id, event_id) here without an issue. I wasn't sure if this was an oversight or an explicit reason to be done this way.

Turns out, a user was poking around with the client and explicitly did this. So I don't think we need to be overly defensive here, but it did break the app then to view the event details page. So we should either reject the input, override an existing entry with create_or_update, or just ignore multiple records and choose the latest (what I proposed here before I fully understood what was going on).

@dcramer
Copy link
Member

dcramer commented Apr 29, 2016

lgtm

@mattrobenolt
Copy link
Contributor Author

@dcramer Just updated this to use a UNIQUE constraint instead, and the API now updates an existing one if found, rather than just erroring out all over the place.

@mitsuhiko
Copy link
Member

@mattrobenolt we should merge this. there are already so many migration PRs pending and cooking and this one is basically already signed off.

@mattrobenolt
Copy link
Contributor Author

Yeah, gonna rebase, recreate migration, and merge this now.

@mattrobenolt mattrobenolt force-pushed the user-reports branch 2 times, most recently from 887886d to 73ac551 Compare May 9, 2016 16:38
@mattrobenolt mattrobenolt merged this pull request into master May 9, 2016
@mattrobenolt mattrobenolt deleted the user-reports branch May 9, 2016 17:10
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants