feat(releases): Implement threshold details GET #55969

EricHasegawa · 2023-09-10T22:27:42Z

Basic GET endpoint for release thresholds -> pass in an org id, project id, and threshold id, get back the details of the matching threshold.

codecov · 2023-09-10T22:55:04Z

Codecov Report

Merging #55969 (f67269e) into master (41e8adc) will decrease coverage by 0.01%.
Report is 24 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #55969      +/-   ##
==========================================
- Coverage   78.67%   78.67%   -0.01%     
==========================================
  Files        5068     5070       +2     
  Lines      217677   217936     +259     
  Branches    36841    36881      +40     
==========================================
+ Hits       171254   171453     +199     
- Misses      40900    40958      +58     
- Partials     5523     5525       +2

Files Changed	Coverage
...sentry/api/serializers/models/release_threshold.py	`ø`
.../sentry/api/endpoints/release_threshold_details.py	`100.00%`
src/sentry/api/urls.py	`100.00%`

src/sentry/api/endpoints/release_threshold_details.py

nhsiehgit

+1 pending Cathy's comment

cathteng · 2023-09-12T18:56:17Z

src/sentry/api/endpoints/release_threshold_details.py

+        "GET": ApiPublishStatus.PRIVATE,
+    }
+
+    def get(self, request: Request, project: Project, threshold_id: str) -> HttpResponse:


i am assuming you will have a PUT (and maybe a DELETE), so it might be worth refactoring with a convert_args function so you already have the ReleaseThreshold object you're getting/modifying/deleting

convert_args just allows us to convert the path param (threshold_id) into the actual model instance, correct?

cathteng · 2023-09-12T18:56:54Z

src/sentry/api/endpoints/release_threshold_details.py

+            )
+            return Response(serialize(release_threshold, request.user), status=200)
+        except ReleaseThreshold.DoesNotExist:
+            return Response(status=404)


do you want to raise ResourceDoesNotExist?

schew2381 · 2023-09-13T17:49:39Z

tests/sentry/api/endpoints/test_release_threshold_details.py

+from sentry.testutils.cases import APITestCase
+
+
+class ReleaseThresholdDetailsTest(APITestCase):


nit: switch to using get_success_response and get_error_response over client.get.

Suggested change

class ReleaseThresholdDetailsTest(APITestCase):

class ReleaseThresholdDetailsTest(APITestCase):

endpoint = "sentry-api-0-project-release-thresholds-details"

method = "get"

schew2381 · 2023-09-13T17:51:19Z

tests/sentry/api/endpoints/test_release_threshold_details.py

+        url = reverse(
+            "sentry-api-0-project-release-thresholds-details",
+            kwargs={
+                "organization_slug": self.organization.slug,
+                "project_slug": self.project.slug,
+                "threshold_id": 123,
+            },
+        )
+        response = self.client.get(url)
+
+        assert response.status_code == 404


same nit, can switch to much shorter test request

Suggested change

url = reverse(

"sentry-api-0-project-release-thresholds-details",

kwargs={

"organization_slug": self.organization.slug,

"project_slug": self.project.slug,

"threshold_id": 123,

},

)

response = self.client.get(url)

assert response.status_code == 404

self.get_error_response(self.organization.slug, self.project.slug, 123, status_code=404)

hmm... eh 🤷

I'm indifferent.
Sure it can make the tests shorter, but it makes them more difficult to decipher and obfuscates away what the test is actually testing for

nhsiehgit

comment comment comment

overall +1 pending nits from others

nhsiehgit · 2023-09-13T23:20:30Z

src/sentry/api/endpoints/release_threshold_details.py

+        "GET": ApiPublishStatus.PRIVATE,
+    }
+
+    def get(self, request: Request, project: Project, threshold_id: str) -> HttpResponse:


convert_args just allows us to convert the path param (threshold_id) into the actual model instance, correct?

nhsiehgit · 2023-09-13T23:23:18Z

tests/sentry/api/endpoints/test_release_threshold_details.py

+        url = reverse(
+            "sentry-api-0-project-release-thresholds-details",
+            kwargs={
+                "organization_slug": self.organization.slug,
+                "project_slug": self.project.slug,
+                "threshold_id": 123,
+            },
+        )
+        response = self.client.get(url)
+
+        assert response.status_code == 404


hmm... eh 🤷

I'm indifferent.
Sure it can make the tests shorter, but it makes them more difficult to decipher and obfuscates away what the test is actually testing for

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 10, 2023

vercel bot deployed to Preview September 10, 2023 22:29 View deployment

EricHasegawa marked this pull request as ready for review September 11, 2023 20:30

EricHasegawa requested a review from a team as a code owner September 11, 2023 20:30

EricHasegawa requested review from a team and nhsiehgit September 11, 2023 20:30

cathteng reviewed Sep 11, 2023

View reviewed changes

src/sentry/api/endpoints/release_threshold_details.py Outdated Show resolved Hide resolved

nhsiehgit reviewed Sep 11, 2023

View reviewed changes

EricHasegawa requested review from schew2381, nhsiehgit and cathteng September 12, 2023 17:14

vercel bot deployed to Preview September 12, 2023 17:17 View deployment

cathteng reviewed Sep 12, 2023

View reviewed changes

schew2381 reviewed Sep 13, 2023

View reviewed changes

nhsiehgit approved these changes Sep 13, 2023

View reviewed changes

EricHasegawa added 2 commits September 14, 2023 07:41

feat(releases): Implement threshold details GET

18e4043

address comments

def5205

EricHasegawa force-pushed the EricHasegawa/threshold_details branch from 6a05002 to def5205 Compare September 14, 2023 14:42

vercel bot deployed to Preview September 14, 2023 14:48 View deployment

Use convert args

7e7ebe8

EricHasegawa force-pushed the EricHasegawa/threshold_details branch from 13336c4 to 7e7ebe8 Compare September 14, 2023 15:00

vercel bot deployed to Preview September 14, 2023 15:05 View deployment

fix typing

f373eb6

EricHasegawa force-pushed the EricHasegawa/threshold_details branch from af46777 to f373eb6 Compare September 14, 2023 15:07

vercel bot deployed to Preview September 14, 2023 15:12 View deployment

switch types

f67269e

EricHasegawa force-pushed the EricHasegawa/threshold_details branch from 8c1399b to f67269e Compare September 14, 2023 17:53

vercel bot deployed to Preview September 14, 2023 17:56 View deployment

EricHasegawa merged commit 6df760e into master Sep 14, 2023
50 checks passed

EricHasegawa deleted the EricHasegawa/threshold_details branch September 14, 2023 21:10

github-actions bot locked and limited conversation to collaborators Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(releases): Implement threshold details GET #55969

feat(releases): Implement threshold details GET #55969

EricHasegawa commented Sep 10, 2023

codecov bot commented Sep 10, 2023 •

edited

Loading

nhsiehgit left a comment

cathteng Sep 12, 2023

nhsiehgit Sep 13, 2023

cathteng Sep 12, 2023

schew2381 Sep 13, 2023

schew2381 Sep 13, 2023

nhsiehgit Sep 13, 2023

nhsiehgit left a comment

nhsiehgit Sep 13, 2023

nhsiehgit Sep 13, 2023

		from sentry.testutils.cases import APITestCase


		class ReleaseThresholdDetailsTest(APITestCase):

feat(releases): Implement threshold details GET #55969

feat(releases): Implement threshold details GET #55969

Conversation

EricHasegawa commented Sep 10, 2023

codecov bot commented Sep 10, 2023 • edited Loading

Codecov Report

nhsiehgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsiehgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2023 •

edited

Loading