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

fix(context): Address a few small context bugs #72013

Merged
merged 6 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions static/app/components/events/contexts/contextCard.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import startCase from 'lodash/startCase';
import {EventFixture} from 'sentry-fixture/event';
import {GroupFixture} from 'sentry-fixture/group';
import {ProjectFixture} from 'sentry-fixture/project';

import {render, screen} from 'sentry-test/reactTestingLibrary';

import ContextCard from 'sentry/components/events/contexts/contextCard';
import * as utils from 'sentry/components/events/contexts/utils';

describe('ContextCard', function () {
const group = GroupFixture();
Expand Down Expand Up @@ -42,7 +42,7 @@ describe('ContextCard', function () {
/>
);

expect(screen.getByText(startCase(alias))).toBeInTheDocument();
expect(screen.getByText(alias)).toBeInTheDocument();
Object.entries(simpleContext).forEach(([key, value]) => {
expect(screen.getByText(key)).toBeInTheDocument();
expect(screen.getByText(value)).toBeInTheDocument();
Expand All @@ -57,10 +57,11 @@ describe('ContextCard', function () {

it('renders with icons if able', function () {
const event = EventFixture();
const iconSpy = jest.spyOn(utils, 'getContextIcon');

leeandher marked this conversation as resolved.
Show resolved Hide resolved
const browserContext = {
type: 'browser',
name: 'Firefox',
name: 'firefox',
version: 'Infinity',
};
const browserCard = render(
Expand All @@ -73,7 +74,8 @@ describe('ContextCard', function () {
project={project}
/>
);
expect(screen.getByRole('img')).toBeInTheDocument();
expect(iconSpy.mock.results[0].value.props.name).toBe('firefox');
iconSpy.mockReset();
browserCard.unmount();

const unknownContext = {
Expand All @@ -91,7 +93,7 @@ describe('ContextCard', function () {
project={project}
/>
);
expect(screen.queryByRole('img')).not.toBeInTheDocument();
expect(iconSpy.mock.results[0].value).toBeUndefined();
});

it('renders the annotated text and errors', function () {
Expand Down
10 changes: 8 additions & 2 deletions static/app/components/events/contexts/contextIcon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,18 +138,24 @@ function getLogoImage(name: string) {

type Props = {
name: string;
hideUnknown?: boolean;
size?: IconSize;
};

function ContextIcon({name, size: providedSize = 'xl'}: Props) {
function ContextIcon({name, size: providedSize = 'xl', hideUnknown = false}: Props) {
const theme = useTheme();
const size = theme.iconSizes[providedSize];

// Apply darkmode CSS to icon when in darkmode
const isDarkmode = useLegacyStore(ConfigStore).theme === 'dark';
const extraCass = isDarkmode && INVERT_IN_DARKMODE.includes(name) ? darkCss : null;

return <img height={size} width={size} css={extraCass} src={getLogoImage(name)} />;
const imageName = getLogoImage(name);
if (hideUnknown && imageName === logoUnknown) {
return null;
}
Comment on lines +154 to +156
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@malwilley This is the issue since even though we're asking for a firefox logo, this line will resolve true because the unknown logo and firefox logo both resolve to this for tests. So in this test, the logo doesn't actually render any more, meaning i can't use a test id. I also couldn't find a way to override this but if you know of a better alternative that'd be great


return <img height={size} width={size} css={extraCass} src={imageName} />;
leeandher marked this conversation as resolved.
Show resolved Hide resolved
}

export default ContextIcon;
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ describe('operating system event context', function () {
},
});

expect(screen.getByText('Raw Description')).toBeInTheDocument(); // subject
expect(screen.getByText('raw_description')).toBeInTheDocument(); // subject
await userEvent.hover(screen.getByText(/redacted/));
expect(
await screen.findByText(
Expand Down
4 changes: 2 additions & 2 deletions static/app/components/events/contexts/utils.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ describe('contexts utils', function () {
const unknownData = getUnknownData({allData, knownKeys});

expect(unknownData).toEqual([
{key: 'username', value: 'a', subject: 'Username', meta: undefined},
{key: 'count', value: 1000, subject: 'Count', meta: undefined},
{key: 'username', value: 'a', subject: 'username', meta: undefined},
{key: 'count', value: 1000, subject: 'count', meta: undefined},
]);
});
});
Expand Down
12 changes: 5 additions & 7 deletions static/app/components/events/contexts/utils.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {Fragment} from 'react';
import styled from '@emotion/styled';
import startCase from 'lodash/startCase';
import moment from 'moment-timezone';

import UserAvatar from 'sentry/components/avatar/userAvatar';
Expand All @@ -19,7 +18,6 @@ import type {
Project,
} from 'sentry/types';
import {defined} from 'sentry/utils';
import {toTitleCase} from 'sentry/utils/string/toTitleCase';

import {AppEventContext, getKnownAppContextData, getUnknownAppContextData} from './app';
import {
Expand Down Expand Up @@ -292,7 +290,7 @@ export function getUnknownData({
.map(([key, value]) => ({
key,
value,
subject: startCase(key),
subject: key,
meta: meta?.[key]?.[''],
}));
}
Expand All @@ -311,7 +309,7 @@ export function getContextTitle({
}

if (!defined(type)) {
return toTitleCase(alias);
return alias;
}

switch (type) {
Expand Down Expand Up @@ -346,10 +344,10 @@ export function getContextTitle({
case 'laravel':
return t('Laravel Context');
default:
return toTitleCase(alias);
return alias;
}
default:
return toTitleCase(type);
return type;
}
}

Expand Down Expand Up @@ -406,7 +404,7 @@ export function getContextIcon({
if (iconName.length === 0) {
return null;
}
return <ContextIcon name={iconName} size="sm" />;
return <ContextIcon name={iconName} size="sm" hideUnknown />;
}

export function getFormattedContextData({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {Fragment, isValidElement} from 'react';

import {t} from 'sentry/locale';
import {isEmptyObject} from 'sentry/utils/object/isEmptyObject';

import {Redaction} from './redaction';

Expand All @@ -14,7 +15,7 @@ type Props = {
// first place. It's much more likely that `withMeta` is buggy or improperly
// used than that this component has a bug.
export function ValueElement({value, meta}: Props) {
if (!!value && meta) {
if (!!value && !isEmptyObject(meta)) {
return <Redaction>{value}</Redaction>;
}

Expand Down
Loading