-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(cells) Hide US2 in customer facing dropdowns #116529
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
base: fix-prodeng-1457
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import {ConfigStore} from 'sentry/stores/configStore'; | ||
| import type {Config} from 'sentry/types/system'; | ||
| import {getRegionUrlOptions, getRegionNameOptions} from 'sentry/utils/regions'; | ||
|
|
||
| describe('getRegionUrlOptions', () => { | ||
| let configstate: Config; | ||
|
|
||
| beforeEach(() => { | ||
| configstate = ConfigStore.getState(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| ConfigStore.loadInitialData(configstate); | ||
| }); | ||
|
|
||
| it('filters out excluded names', () => { | ||
| ConfigStore.set('regions', [ | ||
| {name: 'us', url: 'https://us.sentry.io'}, | ||
| {name: 'de', url: 'https://de.sentry.io'}, | ||
| {name: 'ja', url: 'https://ja.sentry.io'}, | ||
| ]); | ||
|
|
||
| const res = getRegionUrlOptions([ | ||
| {name: 'us', url: 'https://us.sentry.io', displayName: 'us'}, | ||
| ]); | ||
| expect(res).toHaveLength(2); | ||
| expect(res[0]).toEqual({ | ||
| value: 'https://de.sentry.io', | ||
| label: '🇪🇺 European Union (EU)', | ||
| }); | ||
| expect(res[1]).toEqual({value: 'https://ja.sentry.io', label: ' ja'}); | ||
|
|
||
| // Excluding the only included option = empty set. | ||
| const none = getRegionUrlOptions( | ||
| [{name: 'us', url: 'https://us.sentry.io', displayName: 'us'}], | ||
| ['us'] | ||
| ); | ||
| expect(none).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('limits to only parameter', () => { | ||
| ConfigStore.set('regions', [ | ||
| {name: 'us', url: 'https://us.sentry.io'}, | ||
| {name: 'de', url: 'https://de.sentry.io'}, | ||
| {name: 'ja', url: 'https://ja.sentry.io'}, | ||
| ]); | ||
|
|
||
| const res = getRegionUrlOptions([], ['us']); | ||
| expect(res).toHaveLength(1); | ||
| expect(res[0]).toEqual({ | ||
| value: 'https://us.sentry.io', | ||
| label: '🇺🇸 United States of America (US)', | ||
| }); | ||
| }); | ||
|
|
||
| it('always excludes US2', () => { | ||
| ConfigStore.set('regions', [ | ||
| {name: 'us', url: 'https://us.sentry.io'}, | ||
| {name: 'us2', url: 'https://us2.sentry.io'}, | ||
| {name: 'de', url: 'https://de.sentry.io'}, | ||
| {name: 'ja', url: 'https://ja.sentry.io'}, | ||
| ]); | ||
|
|
||
| const res = getRegionUrlOptions(); | ||
| expect(res).toHaveLength(3); | ||
| res.forEach(item => expect(item.value).not.toContain('us2')); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getRegionNameOptions', () => { | ||
| let configstate: Config; | ||
|
|
||
| beforeEach(() => { | ||
| configstate = ConfigStore.getState(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| ConfigStore.loadInitialData(configstate); | ||
| }); | ||
| it('always excludes US2', () => { | ||
| ConfigStore.set('regions', [ | ||
| {name: 'us', url: 'https://us.sentry.io'}, | ||
| {name: 'us2', url: 'https://us2.sentry.io'}, | ||
| {name: 'de', url: 'https://de.sentry.io'}, | ||
| {name: 'ja', url: 'https://ja.sentry.io'}, | ||
| ]); | ||
|
|
||
| const res = getRegionNameOptions(); | ||
| expect(res).toHaveLength(3); | ||
|
|
||
| expect(res[0]).toEqual({value: 'us', label: '🇺🇸 United States of America (US)'}); | ||
|
|
||
| res.forEach(item => expect(item.label).not.toContain('us2')); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import {Redirect} from 'sentry/components/redirect'; | |
| import {SentryDocumentTitle} from 'sentry/components/sentryDocumentTitle'; | ||
| import {IconArrow} from 'sentry/icons'; | ||
| import {t} from 'sentry/locale'; | ||
| import {ConfigStore} from 'sentry/stores/configStore'; | ||
| import {getRegionUrlOptions} from 'sentry/utils/regions'; | ||
| import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; | ||
| import {useApi} from 'sentry/utils/useApi'; | ||
| import {useNavigate} from 'sentry/utils/useNavigate'; | ||
|
|
@@ -76,7 +76,7 @@ export function RelocationOnboarding() { | |
| const stepObj = onboardingSteps.find(({id}) => stepId === id); | ||
| const stepIndex = onboardingSteps.findIndex(({id}) => stepId === id); | ||
| const api = useApi(); | ||
| const regions = ConfigStore.get('regions'); | ||
| const regionOptions = getRegionUrlOptions(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. US2 excluded from relocation API data-fetching callsHigh Severity
Additional Locations (2)Reviewed by Cursor Bugbot for commit a57167b. Configure here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The new Suggested FixThe Prompt for AI AgentAlso affects:
Did we get this right? 👍 / 👎 to inform future reviews. |
||
| const [existingRelocationState, setExistingRelocationState] = useState( | ||
| LoadingState.FETCHING | ||
| ); | ||
|
|
@@ -95,10 +95,10 @@ export function RelocationOnboarding() { | |
| const fetchExistingRelocation = useCallback(() => { | ||
| setExistingRelocationState(LoadingState.FETCHING); | ||
| return Promise.all( | ||
| regions.map(region => | ||
| regionOptions.map(option => | ||
| api.requestPromise('/relocations/', { | ||
| method: 'GET', | ||
| host: region.url, | ||
| host: option.value, | ||
| }) | ||
| ) | ||
| ) | ||
|
|
@@ -142,7 +142,7 @@ export function RelocationOnboarding() { | |
| setExistingRelocation(''); | ||
| setExistingRelocationState(LoadingState.ERROR); | ||
| }); | ||
| }, [api, navigate, regions, relocationState, stepId]); | ||
| }, [api, navigate, regionOptions, relocationState, stepId]); | ||
| useEffect(() => { | ||
| fetchExistingRelocation(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
|
|
@@ -151,17 +151,20 @@ export function RelocationOnboarding() { | |
| const fetchPublicKeys = useCallback(() => { | ||
| setPublicKeysState(LoadingState.FETCHING); | ||
| return Promise.all( | ||
| regions.map(region => | ||
| regionOptions.map(option => | ||
| api.requestPromise('/publickeys/relocations/', { | ||
| method: 'GET', | ||
| host: region.url, | ||
| host: option.value, | ||
| }) | ||
| ) | ||
| ) | ||
| .then(responses => { | ||
| setPublicKeys( | ||
| new Map<string, string>( | ||
| regions.map((region, index) => [region.url, responses[index].public_key]) | ||
| regionOptions.map((option, index) => [ | ||
| option.value, | ||
| responses[index].public_key, | ||
| ]) | ||
| ) | ||
| ); | ||
| setPublicKeysState(LoadingState.FETCHED); | ||
|
|
@@ -170,7 +173,7 @@ export function RelocationOnboarding() { | |
| setPublicKeys(new Map<string, string>()); | ||
| setPublicKeysState(LoadingState.ERROR); | ||
| }); | ||
| }, [api, regions]); | ||
| }, [api, regionOptions]); | ||
| useEffect(() => { | ||
| fetchPublicKeys(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ import {Client} from 'sentry/api'; | |
| import {SelectField} from 'sentry/components/forms/fields/selectField'; | ||
| import type {Organization} from 'sentry/types/organization'; | ||
| import { | ||
| getRegionChoices, | ||
| getRegionUrlOptions, | ||
| getRegionDataFromOrganization, | ||
| getRegions, | ||
| } from 'sentry/utils/regions'; | ||
|
|
@@ -65,14 +65,16 @@ class ForkCustomerActionImpl extends Component<Props> { | |
| render() { | ||
| const {organization} = this.props; | ||
| const currentRegionData = getRegionDataFromOrganization(organization); | ||
| const regionChoices = getRegionChoices(currentRegionData ? [currentRegionData] : []); | ||
| const regionOptions = getRegionUrlOptions( | ||
| currentRegionData ? [currentRegionData] : [] | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Admin fork dropdown incorrectly hides US2 regionMedium Severity The admin "Duplicate into Region" dropdown in Reviewed by Cursor Bugbot for commit a57167b. Configure here. |
||
| return ( | ||
| <Fragment> | ||
| <SelectField | ||
| name="regionUrl" | ||
| label="Duplicate into Region" | ||
| help="Choose which region to duplicate this organization's low volume metadata into. This will kick off a SAAS->SAAS relocation job, but the source organization will not be affected." | ||
| choices={regionChoices} | ||
| options={regionOptions} | ||
| inline={false} | ||
| stacked | ||
| required | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty selectableRegions now shows all regions instead of none
Medium Severity
When
selectableRegionsis an empty array (e.g., whenrelocationConfigis undefined), the old code filtered to an empty list since no region matched the whitelist. The new callgetRegionUrlOptions([], selectableRegions)passes an emptyonlyarray, which triggers theonly.length > 0short-circuit in the filter, causing all regions to be shown. This changes the semantics from "nothing is selectable" to "everything is selectable" when relocation config is missing.Reviewed by Cursor Bugbot for commit a57167b. Configure here.