sentry icon indicating copy to clipboard operation
sentry copied to clipboard

feat(replay): viewed_by_me search alias

Open aliu39 opened this issue 9 months ago • 6 comments

Follow up from https://github.com/getsentry/sentry/issues/64924

On the backend, we query by viewed_by_id. Previously, we expose this in the search bar typeahead. But this doesn't really make sense since you need to be an admin to lookup user ids. So we'll use this for convenience - "have I seen/not seen this?"

aliu39 avatar May 08 '24 20:05 aliu39

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.00%. Comparing base (1f61c87) to head (24655de). Report is 38 commits behind head on master.

:exclamation: Current head 24655de differs from pull request most recent head 6655c40. Consider uploading reports for the commit 6655c40 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #70535       +/-   ##
===========================================
+ Coverage   63.16%   80.00%   +16.83%     
===========================================
  Files        6501     6505        +4     
  Lines      290600   290418      -182     
  Branches    50105    50064       -41     
===========================================
+ Hits       183554   232342    +48788     
+ Misses     106606    57710    -48896     
+ Partials      440      366       -74     
Files Coverage Δ
src/sentry/replays/usecases/query/__init__.py 97.29% <100.00%> (+26.08%) :arrow_up:

... and 2031 files with indirect coverage changes

codecov[bot] avatar May 09 '24 17:05 codecov[bot]

worth trying to get to 100% code coverage on this one, can see which lines are missing it @JoshFerge Looks like it's the request_user_id=None case, which happens when request.user is None -- not sure when this would happen in prod and how to write a test for it. I guess when you view the page without logging into Sentry? Is such a thing possible?

aliu39 avatar May 13 '24 17:05 aliu39

worth trying to get to 100% code coverage on this one, can see which lines are missing it @JoshFerge Looks like it's the request_user_id=None case, which happens when request.user is None -- not sure when this would happen in prod and how to write a test for it. I guess when you view the page without logging into Sentry? Is such a thing possible?

if its impossible, do we need to handle that case?

JoshFerge avatar May 13 '24 17:05 JoshFerge

worth trying to get to 100% code coverage on this one, can see which lines are missing it @JoshFerge Looks like it's the request_user_id=None case, which happens when request.user is None -- not sure when this would happen in prod and how to write a test for it. I guess when you view the page without logging into Sentry? Is such a thing possible?

if its impossible, do we need to handle that case?

I'm not sure if it's possible. The type of request.user is an optional, but let me dig into django docs for this

aliu39 avatar May 13 '24 17:05 aliu39

worth trying to get to 100% code coverage on this one, can see which lines are missing it @JoshFerge Looks like it's the request_user_id=None case, which happens when request.user is None -- not sure when this would happen in prod and how to write a test for it. I guess when you view the page without logging into Sentry? Is such a thing possible?

if its impossible, do we need to handle that case?

I'm not sure if it's possible. The type of request.user is an optional, but let me dig into django docs for this. i don't think it should ever be possible for a non logged in user to make it through auth for this, so at the top of the endpoint you could just say if request.User is None then return Unauthorized, or something like that

JoshFerge avatar May 13 '24 17:05 JoshFerge

worth trying to get to 100% code coverage on this one, can see which lines are missing it @JoshFerge Looks like it's the request_user_id=None case, which happens when request.user is None -- not sure when this would happen in prod and how to write a test for it. I guess when you view the page without logging into Sentry? Is such a thing possible?

if its impossible, do we need to handle that case?

I'm not sure if it's possible. The type of request.user is an optional, but let me dig into django docs for this. i don't think it should ever be possible for a non logged in user to make it through auth for this, so at the top of the endpoint you could just say if request.User is None then return Unauthorized, or something like that

Sounds good!

aliu39 avatar May 13 '24 17:05 aliu39

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

github-actions[bot] avatar May 14 '24 23:05 github-actions[bot]