Skip to content

Commit

Permalink
fix: Can't toggle off from the ad-hoc tools menu if you've toggled on…
Browse files Browse the repository at this point in the history
… in DetailsView (#7431)

#### Details

Two ad-hoc tools (Automated Checks and Needs Review) have the ability to
view issues in DetailsView. Unfortunately, if you try to toggle either
of them off in ad-hoc tools while their corresponding DetailsView list
of issues is open, the DetailsView toggle will overwrite the "off" state
and force it to be "on". Additionally, toggling off in DetailsView does
not turn the toggle off in ad-hoc tools.

This PR keeps these toggles in sync by:
* moving the code that determines if a scan needs to be run based on
state to `componentDidMount` instead of on render.
* triggering
`VisualizationActions.enableVisualization`/`VisualizationActions.disabledVisualization`
when `CardSelectionActions.toggleVisualHelper` is triggered, which
involves:
* making sure `VisualizationActions` are accessible from
`CardSelectionActionCreator`s
* sending a `VisualizationTogglePayload` instead of `BasePayload` with
the enabled state (passed in from the `VisualHelperToggle`'s `onClick`)
* triggering `CardSelectionActions.toggleVisualHelper` when
`VisualizationActions.enableVisualization`/`VisualizationActions.disabledVisualization`
are triggered, which involves:
* checking the value of `payload.test` and triggering
`needsReviewSelectionActions.toggleVisualHelper` or
`cardSelectionActions.toggleVisualHelper` if the `VisualizationType`
matches.

##### Motivation

Addresses issue #6253 

##### Context

<!-- Are there any parts that you've intentionally left out-of-scope for
a later PR to handle? -->

<!-- Were there any alternative approaches you considered? What
tradeoffs did you consider? -->

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: #6253
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.

---------

Co-authored-by: Madalyn Parker <madparker@microsoft.com>
  • Loading branch information
madalynrose and Madalyn Parker committed Aug 29, 2024
1 parent b4de539 commit 9c0dafa
Show file tree
Hide file tree
Showing 15 changed files with 228 additions and 51 deletions.
34 changes: 18 additions & 16 deletions src/DetailsView/components/issues-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,24 @@ export class IssuesTable extends React.Component<IssuesTableProps> {
super(props);
}

componentDidMount(): void {
const cardCount = this.getCardCount();
const assessment = this.props.getProvider().forType(this.props.selectedVisualizationType);
const requirement = assessment?.requirements[0].key;
if (!this.props.issuesEnabled && cardCount > 0) {
this.props.deps.detailsViewActionMessageCreator.enableFastPassVisualHelperWithoutScan(
this.props.selectedVisualizationType,
requirement,
);
}
if (!this.props.issuesEnabled && cardCount === 0) {
this.props.deps.detailsViewActionMessageCreator.rescanVisualizationWithoutTelemetry(
this.props.selectedVisualizationType,
requirement,
);
}
}

public render(): JSX.Element {
return (
<div className={styles.issuesTable}>
Expand Down Expand Up @@ -109,22 +127,6 @@ export class IssuesTable extends React.Component<IssuesTableProps> {
}

private renderComponent(): JSX.Element {
const cardCount = this.getCardCount();
const assessment = this.props.getProvider().forType(this.props.selectedVisualizationType);
const requirement = assessment?.requirements[0].key;
if (!this.props.issuesEnabled && cardCount > 0) {
this.props.deps.detailsViewActionMessageCreator.enableFastPassVisualHelperWithoutScan(
this.props.selectedVisualizationType,
requirement,
);
}
if (!this.props.issuesEnabled && cardCount === 0) {
this.props.deps.detailsViewActionMessageCreator.rescanVisualizationWithoutTelemetry(
this.props.selectedVisualizationType,
requirement,
);
}

if (this.props.scanning) {
return this.renderSpinner('Scanning...');
}
Expand Down
10 changes: 10 additions & 0 deletions src/background/actions/action-creator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,16 @@ export class ActionCreator {
this.executingScope,
);
}
switch (payload.test) {
case VisualizationType.Issues:
await this.cardSelectionActions.toggleVisualHelper.invoke(null);
break;
case VisualizationType.NeedsReview:
await this.needsReviewCardSelectionActions.toggleVisualHelper.invoke(null);
break;
default:
break;
}
};

private onRescanVisualization = async (payload: RescanVisualizationPayload) => {
Expand Down
10 changes: 9 additions & 1 deletion src/background/actions/card-selection-action-creator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import { VisualizationActions } from 'background/actions/visualization-actions';
import { StoreNames } from 'common/stores/store-names';

import * as TelemetryEvents from '../../common/extension-telemetry-events';
Expand All @@ -11,12 +12,14 @@ import {
BaseActionPayload,
CardSelectionPayload,
RuleExpandCollapsePayload,
VisualizationTogglePayload,
} from './action-payloads';

export class CardSelectionActionCreator {
constructor(
private readonly interpreter: Interpreter,
private readonly cardSelectionActions: CardSelectionActions,
private readonly visualizationActions: VisualizationActions,
private readonly telemetryEventHandler: TelemetryEventHandler,
) {}

Expand Down Expand Up @@ -67,8 +70,13 @@ export class CardSelectionActionCreator {
);
};

private onToggleVisualHelper = async (payload: BaseActionPayload): Promise<void> => {
private onToggleVisualHelper = async (payload: VisualizationTogglePayload): Promise<void> => {
await this.cardSelectionActions.toggleVisualHelper.invoke(null);
if (payload.enabled) {
await this.visualizationActions.disableVisualization.invoke(payload.test);
} else {
await this.visualizationActions.enableVisualization.invoke(payload);
}
this.telemetryEventHandler.publishTelemetry(TelemetryEvents.VISUAL_HELPER_TOGGLED, payload);
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import { NeedsReviewCardSelectionActions } from 'background/actions/needs-review-card-selection-actions';
import { VisualizationActions } from 'background/actions/visualization-actions';
import { StoreNames } from 'common/stores/store-names';

import * as TelemetryEvents from '../../common/extension-telemetry-events';
Expand All @@ -11,12 +12,14 @@ import {
BaseActionPayload,
CardSelectionPayload,
RuleExpandCollapsePayload,
VisualizationTogglePayload,
} from './action-payloads';

export class NeedsReviewCardSelectionActionCreator {
constructor(
private readonly interpreter: Interpreter,
private readonly needsReviewCardSelectionActions: NeedsReviewCardSelectionActions,
private readonly visualizationActions: VisualizationActions,
private readonly telemetryEventHandler: TelemetryEventHandler,
) {}

Expand Down Expand Up @@ -69,8 +72,13 @@ export class NeedsReviewCardSelectionActionCreator {
);
};

private onToggleVisualHelper = async (payload: BaseActionPayload): Promise<void> => {
private onToggleVisualHelper = async (payload: VisualizationTogglePayload): Promise<void> => {
await this.needsReviewCardSelectionActions.toggleVisualHelper.invoke(null);
if (payload.enabled) {
await this.visualizationActions.disableVisualization.invoke(payload.test);
} else {
await this.visualizationActions.enableVisualization.invoke(payload);
}
this.telemetryEventHandler.publishTelemetry(TelemetryEvents.VISUAL_HELPER_TOGGLED, payload);
};

Expand Down
2 changes: 2 additions & 0 deletions src/background/tab-context-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,13 @@ export class TabContextFactory {
const cardSelectionActionCreator = new CardSelectionActionCreator(
interpreter,
actionsHub.cardSelectionActions,
actionsHub.visualizationActions,
this.telemetryEventHandler,
);
const needsReviewCardSelectionActionCreator = new NeedsReviewCardSelectionActionCreator(
interpreter,
actionsHub.needsReviewCardSelectionActions,
actionsHub.visualizationActions,
this.telemetryEventHandler,
);
const injectionActionCreator = new InjectionActionCreator(
Expand Down
7 changes: 6 additions & 1 deletion src/common/components/cards/visual-helper-toggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ export type VisualHelperToggleProps = {
export const VisualHelperToggle = NamedFC<VisualHelperToggleProps>('VisualHelperToggle', props => {
return (
<Toggle
onClick={props.cardSelectionMessageCreator.toggleVisualHelper}
onClick={event => {
props.cardSelectionMessageCreator.toggleVisualHelper(
event,
props.visualHelperEnabled,
);
}}
label="Visual helper"
checked={props.visualHelperEnabled}
className={css(styles.visualHelperToggle, props.className)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import {
BaseActionPayload,
CardSelectionPayload,
RuleExpandCollapsePayload,
VisualizationTogglePayload,
} from 'background/actions/action-payloads';
import { TelemetryEventSource } from 'common/extension-telemetry-events';
import { CardSelectionMessageCreator } from 'common/message-creators/card-selection-message-creator';
import { ActionMessageDispatcher } from 'common/message-creators/types/dispatcher';
import { Messages } from 'common/messages';
import { SupportedMouseEvent, TelemetryDataFactory } from 'common/telemetry-data-factory';
import { VisualizationType } from 'common/types/visualization-type';

export class AutomatedChecksCardSelectionMessageCreator implements CardSelectionMessageCreator {
constructor(
Expand Down Expand Up @@ -69,9 +71,14 @@ export class AutomatedChecksCardSelectionMessageCreator implements CardSelection
});
};

public toggleVisualHelper = (event: SupportedMouseEvent) => {
const payload: BaseActionPayload = {
telemetry: this.telemetryFactory.withTriggeredByAndSource(event, this.source),
public toggleVisualHelper = (event: SupportedMouseEvent, isEnabled: boolean) => {
const payload: VisualizationTogglePayload = {
enabled: isEnabled,
test: VisualizationType.Issues,
telemetry: {
...this.telemetryFactory.withTriggeredByAndSource(event, this.source),
enabled: isEnabled,
},
};

this.dispatcher.dispatchMessage({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ export interface CardSelectionMessageCreator {
toggleRuleExpandCollapse: (ruleId: string, event: React.SyntheticEvent) => void;
collapseAllRules: (event: SupportedMouseEvent) => void;
expandAllRules: (event: SupportedMouseEvent) => void;
toggleVisualHelper: (event: SupportedMouseEvent) => void;
toggleVisualHelper: (event: SupportedMouseEvent, isEnabled?: boolean) => void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import {
BaseActionPayload,
CardSelectionPayload,
RuleExpandCollapsePayload,
VisualizationTogglePayload,
} from 'background/actions/action-payloads';
import { TelemetryEventSource } from 'common/extension-telemetry-events';
import { CardSelectionMessageCreator } from 'common/message-creators/card-selection-message-creator';
import { ActionMessageDispatcher } from 'common/message-creators/types/dispatcher';
import { Messages } from 'common/messages';
import { SupportedMouseEvent, TelemetryDataFactory } from 'common/telemetry-data-factory';
import { VisualizationType } from 'common/types/visualization-type';

export class NeedsReviewCardSelectionMessageCreator implements CardSelectionMessageCreator {
constructor(
Expand Down Expand Up @@ -69,11 +71,15 @@ export class NeedsReviewCardSelectionMessageCreator implements CardSelectionMess
});
};

public toggleVisualHelper = (event: SupportedMouseEvent) => {
const payload: BaseActionPayload = {
telemetry: this.telemetryFactory.withTriggeredByAndSource(event, this.source),
public toggleVisualHelper = (event: SupportedMouseEvent, isEnabled: boolean) => {
const payload: VisualizationTogglePayload = {
test: VisualizationType.NeedsReview,
enabled: isEnabled,
telemetry: {
...this.telemetryFactory.withTriggeredByAndSource(event, this.source),
enabled: isEnabled,
},
};

this.dispatcher.dispatchMessage({
messageType: Messages.NeedsReviewCardSelection.ToggleVisualHelper,
payload,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ describe('IssuesTableTest', () => {

it('includes subtitle if specified', () => {
const props = new TestPropsBuilder()
.setDeps(deps)
.setGetProviderMock(getProviderMock)
.setSubtitle(<>test subtitle text</>)
.build();
Expand Down
Loading

0 comments on commit 9c0dafa

Please sign in to comment.