Implement 'New Threshold' creation by nhsiehgit · Pull Request #58966 · getsentry/sentry

nhsiehgit · 2023-10-27T21:05:17Z

Implements ability to create new thresholds without an existing threshold

Oct-27-2023.14-29-34.mp4

schew2381

I'm not great at frontend, so prob want some more 👀 on this. Also I noticed that thresholds can have multiple conditions and we can add to existing ones on the page. Should we also allow that when creating new ones?

schew2381 · 2023-10-27T22:20:12Z

Would it be better to put these into constants? It's a little difficult to tell what's what, even with the comment explanation above

hmmm i'm not sure if sticking these into a constant would make it anymore clear either :/

schew2381 · 2023-10-27T22:26:26Z

When you fetch all environments, would you want to exclude environments that already have a threshold?

In the example video, we show the production env but there's an existing threshold on the page for it

Nah - it's fine if they want to select an env with existing thresholds still. It'll still work as expected

sentaur-athena · 2023-10-27T22:07:18Z

Product question, can you select multiple projects? The selector is multi-select and selecting just one makes it confusing.

Edit: I saw lower down in the code that it's actually multi select. Where does this tooltip show up?

Yup - this tooltip shows up on the "create threshold" button.
I added a video to the description so you can see it there

sentaur-athena · 2023-10-27T22:08:10Z

sentaur-athena · 2023-10-27T22:17:59Z

'' is not really being used. How about to turn this into a one liner?
return threshold.environment?.name ? selection.environments.indexOf(envName) > -1 : !selection.environments.length

My point is envName could just be threshold.environment?.name this if is not doing much.

Ah that's a good callout 👍

sentaur-athena · 2023-10-27T22:25:38Z

Could this be empty at this point?

Yes, if this is empty then we won't render anything

I mean newThresholdGroup[0]. If it can be empty maybe newThresholdGroup && newThresholdGroup[0]

Oh - so it'll always be an array (never falsey)
If it's an empty array, then we want this to be false, so no need to check for existence of newThresholdGroup

scttcper · 2023-10-30T18:46:01Z

+              disabled={newThresholdDisabled}
+            >
+              <IconAdd isCircled /> &nbsp; {t('New Threshold')}


button takes an icon

Suggested change

disabled={newThresholdDisabled}

>

<IconAdd isCircled />   {t('New Threshold')}

disabled={newThresholdDisabled}

icon={<IconAdd isCircled />}

>

{t('New Threshold')}

scttcper · 2023-10-30T18:53:05Z

+      setNewThresholdGroup([
+        ...newThresholdGroup,
+        {
+          id: `${NEW_GROUP_PREFIX}-${newGroupIterator}`,


you might be able to just add a unique id instead of tracking how many there have been https://github.com/getsentry/sentry/blob/master/static/app/utils/guid.tsx#L6

Suggested change

id: `${NEW_GROUP_PREFIX}-${newGroupIterator}`,

id: `${NEW_GROUP_PREFIX}-${newGroupIterator}`,

Ah yea - this isn't really to track how many, but just an easy way to ensure separate keys.
def could generate unique id's but didn't wnat to bother importing another another package for something that's not even going to be utilized.
The ID here isn't going to be saved at all. This is purely just for rendering

small nit fixes - update button with icon - disable new threshold button when all projects is selected quick follow up from #58966

nhsiehgit requested a review from a team October 27, 2023 21:05

nhsiehgit requested a review from a team as a code owner October 27, 2023 21:05

github-actions Bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Oct 27, 2023

nhsiehgit marked this pull request as draft October 27, 2023 21:06

vercel Bot deployed to Preview October 27, 2023 21:10 View deployment

nhsiehgit force-pushed the new_threshold_btn branch from 5ae49b6 to 5eb195f Compare October 27, 2023 21:26

vercel Bot deployed to Preview October 27, 2023 21:30 View deployment

nhsiehgit marked this pull request as ready for review October 27, 2023 21:30

nhsiehgit requested review from schew2381 and sentaur-athena October 27, 2023 21:30

vercel Bot deployed to Preview October 27, 2023 21:34 View deployment

vercel Bot deployed to Preview October 27, 2023 21:52 View deployment

schew2381 reviewed Oct 27, 2023

View reviewed changes

sentaur-athena reviewed Oct 27, 2023

View reviewed changes

vercel Bot deployed to Preview October 30, 2023 15:36 View deployment

nhsiehgit added 7 commits October 30, 2023 08:42

add new threshold button

49695f3

implement new threshold group logic & callbacks

58909b7

table empty if no new threshold groups

475056c

only show new groups for project selected

60cc936

remove irrelevant note

f969406

lint

6ce9eef

PR nits

3331f03

nhsiehgit force-pushed the new_threshold_btn branch from 69ca027 to 3331f03 Compare October 30, 2023 15:42

vercel Bot deployed to Preview October 30, 2023 15:45 View deployment

nhsiehgit requested review from a team, schew2381 and sentaur-athena October 30, 2023 16:24

getsentry deleted a comment from github-actions Bot Oct 30, 2023

schew2381 removed the Scope: Backend Automatically applied to PRs that change backend components label Oct 30, 2023

sentaur-athena approved these changes Oct 30, 2023

View reviewed changes

nhsiehgit merged commit 2709d9a into master Oct 30, 2023

nhsiehgit deleted the new_threshold_btn branch October 30, 2023 18:44

scttcper reviewed Oct 30, 2023

View reviewed changes

nhsiehgit mentioned this pull request Oct 31, 2023

new threshold nit clean up #59128

Merged

nhsiehgit added a commit that referenced this pull request Oct 31, 2023

new threshold nit clean up (#59128)

3a1bc41

small nit fixes - update button with icon - disable new threshold button when all projects is selected quick follow up from #58966

github-actions Bot locked and limited conversation to collaborators Nov 16, 2023

	id: `${NEW_GROUP_PREFIX}-${newGroupIterator}`,
	id: `${NEW_GROUP_PREFIX}-${newGroupIterator}`,

Uh oh!

Conversation

nhsiehgit commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schew2381 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schew2381 Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scttcper Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nhsiehgit commented Oct 27, 2023 •

edited

Loading

schew2381 left a comment •

edited

Loading

schew2381 Oct 27, 2023 •

edited

Loading

scttcper Oct 30, 2023 •

edited

Loading