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

Show total number of votes and maximum number of votes for all users #488

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ You can find the changelog of the Retrospective Extension below.

_PS: Unfortunately, changelog before v1.0.46 is not available_ 🤦‍♂️

## v1.90.X

* Total number of team votes is now shown together with your own vote count. From [GitHub PR #488](https://github.com/microsoft/vsts-extension-retrospectives/pull/488).
Copy link
Member

Choose a reason for hiding this comment

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

👍


## v1.90.3

* Github Experience: Updating README to include the code coverage and status badges. Github PRs now get code coverage comments from CodeCov. From [Github PR #461](https://github.com/microsoft/vsts-extension-retrospectives/pull/461).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@ const CommonServiceIds = {
ProjectPageService: "ms.vss-tfs-web.tfs-page-data-service"
};

const mockExtensionDataManager = {
getDocuments: () => {},
getDocument: () => {}
};

const mockExtensionDataService = {
getExtensionDataManager: () => {},
getExtensionDataManager: () => mockExtensionDataManager,
};

const mockLocationService = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@

import { mocked } from 'jest-mock';
import { WorkflowPhase } from '../../../interfaces/workItem';
import { TooltipOverflowMode } from 'office-ui-fabric-react';

export const testTeamId = "mocked-team-uuid";
export const testBoardId = "mocked-board-uuid";
Expand Down Expand Up @@ -90,7 +89,7 @@ testColumnsObj[testColumnUuidOne] = {
testColumnsObj[testColumnUuidTwo] = {
columnProperties:
{
id: TooltipOverflowMode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why this was using this random import, but it seemed like a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Appreciate the diligence! Always appreciated :D

id: testColumnUuidTwo,
title: testColumnTwoTitle,
iconClass: 'far fa-smile',
accentColor: '#008100',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,115 @@
import React from 'react';
import { IFeedbackBoardDocument } from '../../interfaces/feedback';
import { shallow } from 'enzyme';
import { mocked } from 'jest-mock';
import FeedbackBoard, { FeedbackBoardProps } from '../../components/feedbackBoard';
import { testColumnProps } from '../__mocks__/mocked_components/mockedFeedbackColumn';
import { IdentityRef } from 'azure-devops-extension-api/WebApi';

jest.mock('../feedbackBoardMetadataForm', () => mocked({}));
jest.mock('azure-devops-extension-api/Work/WorkClient', () => {
const getTeamIterationsMock = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These mocks are copied from feedbackBoardContainer.test.tsx. I don't know if they're general enough to be moved to the __mocks__ folder. It might also be possible to use a simplified version in these tests.

Copy link
Member

Choose a reason for hiding this comment

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

If this chunk is a copy of the other, I think that's a valid thing to move to __mocks__ folder so there's less duplicate code. It's probably okay to remove any bit that's not explicitly called, but that may be worth doing a local test or two to be sure.

return [
mocked({
attributes: mocked({
finishDate: new Date(),
startDate: new Date(),
timeFrame: 1,
}),
id: 'iterationId',
name: 'iteration name',
path: 'default path',
_links: [],
url: 'https://teamfieldvaluesurl'
})
];
};

const getTeamFieldValuesMock = () => {
return [
mocked({
defaultValue: 'default field value',
field: mocked({
referenceName: 'default reference name',
url: 'https://fieldurl'
}),
values: [
mocked({
includeChildren: false,
value: 'default team field value',
})
],
links: [],
url: 'https://teamfieldvaluesurl'
})]
};

const workRestClientMock = jest.fn().mockImplementation(() => ({
getTeamIterations: getTeamIterationsMock,
getTeamFieldValues: getTeamFieldValuesMock,
}));

return { WorkRestClient: workRestClientMock };
});

jest.mock('azure-devops-extension-api/Common', () => ({
getClient: (clientClass: typeof Object) => new clientClass(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could be moved into __mocks__/azure-devops-extension-api/Common/Common.tsx, but there are some of the other tests that depend on the current behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I'll make a more overarching issue to maybe get the tests and components tidied eventually - may be a bit of an undertaking. For now, you should be good to go!

}));

const mockedIdentity: IdentityRef = {
directoryAlias: '',
id: '',
imageUrl: '',
inactive: false,
isAadIdentity: false,
isContainer: false,
isDeletedInOrigin: false,
profileUrl: '',
uniqueName: '',
_links: undefined,
descriptor: '',
displayName: '',
url: ''
};

const mockedBoard: IFeedbackBoardDocument = {
id: testColumnProps.boardId,
title: testColumnProps.boardTitle,
teamId: testColumnProps.team.id,
createdBy: mockedIdentity,
createdDate: new Date(),
columns: testColumnProps.columnIds.map((columnId) => testColumnProps.columns[columnId].columnProperties),
activePhase: 'Vote',
maxVotesPerUser: 5,
boardVoteCollection: {},
teamEffectivenessMeasurementVoteCollection: []
};

const mockedProps: FeedbackBoardProps = {
displayBoard: true,
board: mockedBoard,
team: testColumnProps.team,
workflowPhase: 'Vote',
nonHiddenWorkItemTypes: testColumnProps.nonHiddenWorkItemTypes,
allWorkItemTypes: testColumnProps.allWorkItemTypes,
isAnonymous: testColumnProps.isBoardAnonymous,
hideFeedbackItems: testColumnProps.hideFeedbackItems,
isCarouselDialogHidden: false,
hideCarouselDialog: jest.fn(() => { }),
userId: ''
};

describe(`The Feedback Board Component`, () => {
it(`Should not regress`, () => {});
});
it(`can be rendered`, () => {
const wrapper = shallow(<FeedbackBoard {...mockedProps} />);
const component = wrapper.children().dive();
expect(component.prop('className')).toBe('feedback-board');
});

it(`shows correct vote counts`, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it(`shows correct vote counts`, () => {
it(`shows correct vote counts when no votes have been cast`, () => {

optional: a test for when one or two different people have cast their votes? If not, this can have a TODO or something similar and can be tackled in a later code snippet.

const wrapper = shallow(<FeedbackBoard {...mockedProps} />);
const component = wrapper.children().dive();
expect(component.findWhere((child) => child.prop("className") === "feedback-maxvotes-per-user").text())
.toBe('Votes Used: 0 / 5 (me), 0 / 0 (team)');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ jest.mock('azure-devops-extension-api/WorkItemTracking', () => { });
jest.mock('azure-devops-extension-api/WorkItemTracking/WorkItemTracking', () => { });
jest.mock('azure-devops-extension-api/WorkItemTracking/WorkItemTrackingClient', () => {
const mockWorkItemTrackingClient = {
WorkItemTrackingRestClient: {},
WorkItemTrackingRestClient: jest.fn().mockImplementation(() => ({})),
};

return mockWorkItemTrackingClient;
Expand Down
17 changes: 13 additions & 4 deletions RetrospectiveExtension.Frontend/components/feedbackBoard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class FeedbackBoard extends React.Component<FeedbackBoardProps, FeedbackBoardSta
defaultActionItemIteration: '',
hasItems: false,
isDataLoaded: false,
currentVoteCount: (props.board.boardVoteCollection === undefined || props.board.boardVoteCollection === null) ? "0" : (props.board.boardVoteCollection[this.props.userId] === undefined || props.board.boardVoteCollection[this.props.userId] === null) ? "0" : props.board.boardVoteCollection[this.props.userId]?.toString()
currentVoteCount: props.board.boardVoteCollection?.[this.props.userId]?.toString() ?? "0",
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for trimming this down!

};
}

Expand Down Expand Up @@ -421,19 +421,28 @@ class FeedbackBoard extends React.Component<FeedbackBoardProps, FeedbackBoardSta
onVoteCasted: () => {
itemDataService.getBoardItem(this.props.team.id, this.props.board.id).then((boardItem: IFeedbackBoardDocument) => {
const voteCollection = boardItem.boardVoteCollection;

this.setState({ currentVoteCount: voteCollection === undefined ? "0" : voteCollection[this.props.userId] === undefined ? "0" : voteCollection[this.props.userId].toString() });
this.setState({ currentVoteCount: voteCollection?.[this.props.userId]?.toString() ?? "0"});
});
},
groupTitles: []
};
});

const columnItems = this.state.columnIds
.flatMap((columnId) => this.state.columns[columnId].columnItems);
const participants = new Set(columnItems.flatMap((columnItem) => Object.keys(columnItem.feedbackItem.voteCollection))).size;

const teamVotes = columnItems
Copy link
Member

Choose a reason for hiding this comment

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

I love this solution! Great job!

.flatMap((columnItem) => Object.values(columnItem.feedbackItem.voteCollection))
.reduce((a, b) => a + b, 0)
.toString();
const maxTeamVotes = (participants * this.props.board?.maxVotesPerUser).toString();

return (
<div className="feedback-board">
{this.props.workflowPhase === WorkflowPhase.Vote &&
<div className="feedback-maxvotes-per-user">
<label>Votes Used: {this.state.currentVoteCount} / {this.props.board?.maxVotesPerUser?.toString()}</label>
<label>Votes Used: {this.state.currentVoteCount} / {this.props.board?.maxVotesPerUser?.toString()} (me), {teamVotes} / {maxTeamVotes} (team)</label>
</div>
}
<div className="feedback-columns-container">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class WorkService {
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
catch (e: any) {
if (e.serverError.typeKey === 'CurrentIterationDoesNotExistException') {
if (e.serverError?.typeKey === 'CurrentIterationDoesNotExistException') {
// TODO: Enable once trackTrace is supported
// appInsightsClient.trackTrace(TelemetryExceptions.CurrentTeamIterationNotFound, e);
}
Expand Down