Commit 76d514eb authored by Martin Wortschack's avatar Martin Wortschack

Merge branch '213905-fe-visually-depict-value-stream-analytics-stages-as-a-flow' into 'master'

Resolve "[FE] Visually depict Value Stream Analytics stages as a flow"

See merge request gitlab-org/gitlab!31069
parents 23346d01 f0f3a4c9
......@@ -16,6 +16,7 @@ import { toYmd } from '../../shared/utils';
import RecentActivityCard from './recent_activity_card.vue';
import StageTableNav from './stage_table_nav.vue';
import CustomStageForm from './custom_stage_form.vue';
import PathNavigation from './path_navigation.vue';
export default {
name: 'CycleAnalytics',
......@@ -31,6 +32,7 @@ export default {
RecentActivityCard,
CustomStageForm,
StageTableNav,
PathNavigation,
},
mixins: [glFeatureFlagsMixin(), UrlSyncMixin],
props: {
......@@ -83,6 +85,7 @@ export default {
'selectedProjectIds',
'enableCustomOrdering',
'cycleAnalyticsRequestParams',
'pathNavigationData',
]),
...mapGetters('customStages', ['customStageFormActive']),
shouldRenderEmptyState() {
......@@ -97,6 +100,9 @@ export default {
shouldDisplayTypeOfWorkCharts() {
return !this.hasNoAccessError && !this.isLoading;
},
shouldDsiplayPathNavigation() {
return this.featureFlags.hasPathNavigation && !this.hasNoAccessError;
},
isLoadingTypeOfWork() {
return this.isLoadingTasksByTypeChartTopLabels || this.isLoadingTasksByTypeChart;
},
......@@ -122,6 +128,7 @@ export default {
this.setFeatureFlags({
hasDurationChart: this.glFeatures.cycleAnalyticsScatterplotEnabled,
hasDurationChartMedian: this.glFeatures.cycleAnalyticsScatterplotMedianEnabled,
hasPathNavigation: this.glFeatures.valueStreamAnalyticsPathNavigation,
});
},
methods: {
......@@ -198,38 +205,47 @@ export default {
<h3>{{ __('Value Stream Analytics') }}</h3>
</div>
<div class="mw-100">
<div
class="mt-3 py-2 px-3 d-flex bg-gray-light border-top border-bottom flex-column flex-md-row justify-content-between"
>
<groups-dropdown-filter
v-if="!hideGroupDropDown"
class="js-groups-dropdown-filter dropdown-select"
:query-params="$options.groupsQueryParams"
:default-group="selectedGroup"
@selected="onGroupSelect"
/>
<projects-dropdown-filter
v-if="shouldDisplayFilters"
:key="selectedGroup.id"
class="js-projects-dropdown-filter ml-md-1 mt-1 mt-md-0 dropdown-select"
:group-id="selectedGroup.id"
:query-params="$options.projectsQueryParams"
:multi-select="$options.multiProjectSelect"
:default-projects="selectedProjects"
@selected="onProjectsSelect"
/>
<div
v-if="shouldDisplayFilters"
class="ml-0 ml-md-auto mt-2 mt-md-0 d-flex flex-column flex-md-row align-items-md-center justify-content-md-end"
>
<date-range
:start-date="startDate"
:end-date="endDate"
:max-date-range="$options.maxDateRange"
:include-selected-date="true"
class="js-daterange-picker"
@change="setDateRange"
<div class="mt-3 py-2 px-3 bg-gray-light border-top border-bottom">
<div v-if="shouldDsiplayPathNavigation" class="w-100 pb-2">
<path-navigation
class="js-path-navigation"
:loading="isLoading"
:stages="pathNavigationData"
:selected-stage="selectedStage"
@selected="onStageSelect"
/>
</div>
<div class="d-flex flex-column flex-md-row justify-content-between">
<groups-dropdown-filter
v-if="!hideGroupDropDown"
class="js-groups-dropdown-filter dropdown-select"
:query-params="$options.groupsQueryParams"
:default-group="selectedGroup"
@selected="onGroupSelect"
/>
<projects-dropdown-filter
v-if="shouldDisplayFilters"
:key="selectedGroup.id"
class="js-projects-dropdown-filter ml-0 mt-1 mt-md-0 dropdown-select"
:group-id="selectedGroup.id"
:query-params="$options.projectsQueryParams"
:multi-select="$options.multiProjectSelect"
:default-projects="selectedProjects"
@selected="onProjectsSelect"
/>
<div
v-if="shouldDisplayFilters"
class="ml-0 ml-md-auto mt-2 mt-md-0 d-flex flex-column flex-md-row align-items-md-center justify-content-md-end"
>
<date-range
:start-date="startDate"
:end-date="endDate"
:max-date-range="$options.maxDateRange"
:include-selected-date="true"
class="js-daterange-picker"
@change="setDateRange"
/>
</div>
</div>
</div>
</div>
......
<script>
import { GlPath, GlSkeletonLoading } from '@gitlab/ui';
import { PATH_BACKGROUND_COLOR } from '../constants';
export default {
name: 'PathNavigation',
components: {
GlPath,
GlSkeletonLoading,
},
props: {
loading: {
type: Boolean,
required: false,
default: false,
},
stages: {
type: Array,
required: true,
},
selectedStage: {
type: Object,
required: false,
default: () => {},
},
},
backgroundColor: PATH_BACKGROUND_COLOR,
};
</script>
<template>
<gl-skeleton-loading v-if="loading" :lines="2" class="h-auto pt-2 pb-1" />
<gl-path
v-else
:key="selectedStage.id"
:items="stages"
:background-color="$options.backgroundColor"
@selected="$emit('selected', $event)"
/>
</template>
import { __ } from '~/locale';
import { capitalizeFirstCharacter } from '~/lib/utils/text_utility';
export const PROJECTS_PER_PAGE = 50;
......@@ -35,6 +36,7 @@ export const DEFAULT_STAGE_NAMES = [...Object.keys(EMPTY_STAGE_TEXT), 'total'];
export const TASKS_BY_TYPE_SUBJECT_ISSUE = 'Issue';
export const TASKS_BY_TYPE_SUBJECT_MERGE_REQUEST = 'MergeRequest';
export const TASKS_BY_TYPE_MAX_LABELS = 15;
export const PATH_BACKGROUND_COLOR = '#fafafa'; // $gray-50 (see variables.scss)
export const TASKS_BY_TYPE_SUBJECT_FILTER_OPTIONS = {
[TASKS_BY_TYPE_SUBJECT_ISSUE]: __('Issues'),
......@@ -58,4 +60,18 @@ export const STAGE_ACTIONS = {
export const STAGE_NAME = {
TOTAL: 'total',
PRODUCTION: 'production',
OVERVIEW: 'overview',
};
/**
* An object containing capitalized stages names
* i.e. { TOTAL: 'total' } => { TOTAL: 'Total' }
*/
export const CAPITALIZED_STAGE_NAME = Object.keys(STAGE_NAME).reduce((acc, stage) => {
return {
...acc,
[stage]: capitalizeFirstCharacter(STAGE_NAME[stage]),
};
}, {});
export const PATH_HOME_ICON = 'home';
......@@ -2,6 +2,7 @@ import dateFormat from 'dateformat';
import { isNumber } from 'lodash';
import httpStatus from '~/lib/utils/http_status';
import { dateFormats } from '../../shared/constants';
import { transformStagesForPathNavigation } from '../utils';
export const hasNoAccessError = state => state.errorCode === httpStatus.FORBIDDEN;
......@@ -28,3 +29,16 @@ export const enableCustomOrdering = ({ stages, errorSavingStageOrder }) =>
export const customStageFormActive = ({ isCreatingCustomStage, isEditingCustomStage }) =>
Boolean(isCreatingCustomStage || isEditingCustomStage);
/**
* Until there are controls in place to edit stages outside of the stage table,
* the path navigation component will only display active stages.
*
* https://gitlab.com/gitlab-org/gitlab/-/issues/216227
*/
export const pathNavigationData = ({ stages, medians, selectedStage }) =>
transformStagesForPathNavigation({
stages: filterStagesByHiddenStatus(stages, false),
medians,
selectedStage,
});
import { isNumber } from 'lodash';
import { isNumber, sortBy } from 'lodash';
import dateFormat from 'dateformat';
import { s__, sprintf } from '~/locale';
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import httpStatus from '~/lib/utils/http_status';
import { convertToSnakeCase } from '~/lib/utils/text_utility';
......@@ -12,9 +13,10 @@ import {
getDayDifference,
getDateInPast,
getDateInFuture,
parseSeconds,
} from '~/lib/utils/datetime_utility';
import { dateFormats } from '../shared/constants';
import { STAGE_NAME } from './constants';
import { STAGE_NAME, CAPITALIZED_STAGE_NAME, PATH_HOME_ICON } from './constants';
import { toYmd } from '../shared/utils';
const EVENT_TYPE_LABEL = 'label';
......@@ -341,3 +343,38 @@ export const handleErrorOrRethrow = ({ action, error }) => {
export const isStageNameExistsError = ({ status, errors }) =>
status === httpStatus.UNPROCESSABLE_ENTITY && errors?.name?.includes(ERROR_NAME_RESERVED);
/**
* Takes the stages and median data, combined with the selected stage, to build an
* array which is formatted to proivde the data required for the path navigation.
*
* The stage named 'Total' is renamed to 'Overview', it's configured to have
* the 'home' icon - and is moved to the front of the array.
*
* @param {Array} stages - The stages available to the group / project
* @param {Object} medians - The median values for the stages available to the group / project
* @param {Object} selectedStage - The currently selected stage
* @returns {Array} An array of stages formatted with data required for the path navigation
*/
export const transformStagesForPathNavigation = ({ stages, medians, selectedStage }) => {
const formattedStages = stages.map(stage => {
const { days } = parseSeconds(medians[stage.id], {
daysPerWeek: 7,
hoursPerDay: 24,
limitToDays: true,
});
const isTotalStage = stage.title === CAPITALIZED_STAGE_NAME.TOTAL;
return {
...stage,
metric: days ? sprintf(s__('ValueStreamAnalytics|%{days}d'), { days }) : null,
selected: stage.title === selectedStage.title,
title: isTotalStage ? CAPITALIZED_STAGE_NAME.OVERVIEW : stage.title,
icon: isTotalStage ? PATH_HOME_ICON : null,
};
});
return sortBy(formattedStages, stage =>
stage.title === CAPITALIZED_STAGE_NAME.OVERVIEW ? 0 : 1,
);
};
......@@ -9,6 +9,7 @@ class Analytics::CycleAnalyticsController < Analytics::ApplicationController
before_action do
push_frontend_feature_flag(:cycle_analytics_scatterplot_enabled, default_enabled: true)
push_frontend_feature_flag(:cycle_analytics_scatterplot_median_enabled, default_enabled: true)
push_frontend_feature_flag(:value_stream_analytics_path_navigation)
end
before_action :load_group, only: :show
......
......@@ -11,6 +11,7 @@ class Groups::Analytics::CycleAnalyticsController < Groups::Analytics::Applicati
before_action do
push_frontend_feature_flag(:cycle_analytics_scatterplot_enabled, default_enabled: true)
push_frontend_feature_flag(:cycle_analytics_scatterplot_median_enabled, default_enabled: true)
push_frontend_feature_flag(:value_stream_analytics_path_navigation)
end
before_action :load_group, only: :show
......
......@@ -20,6 +20,7 @@ describe 'Group Value Stream Analytics', :js do
let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha, head_pipeline_of: mr) }
stage_nav_selector = '.stage-nav'
path_nav_selector = '.js-path-navigation'
3.times do |i|
let_it_be("issue_#{i}".to_sym) { create(:issue, title: "New Issue #{i}", project: project, created_at: 2.days.ago) }
......@@ -151,6 +152,21 @@ describe 'Group Value Stream Analytics', :js do
it 'shows the date filter' do
expect(page).to have_selector('.js-daterange-picker', visible: true)
end
it 'does not show the path navigation' do
expect(page).to have_selector(path_nav_selector, visible: false)
end
context 'with path navigation feature flag enabled' do
before do
stub_feature_flags(value_stream_analytics_path_navigation: true)
select_group
end
it 'shows the path navigation' do
expect(page).to have_selector(path_nav_selector, visible: true)
end
end
end
def wait_for_stages_to_load
......@@ -216,6 +232,21 @@ describe 'Group Value Stream Analytics', :js do
end
end
end
context 'path nav' do
before do
stub_feature_flags(value_stream_analytics_path_navigation: true)
end
it 'displays the default list of stages' do
path_nav = page.find(path_nav_selector)
%w[Issue Plan Code Test Review Staging Overview].each do |item|
string_id = "CycleAnalytics|#{item}"
expect(path_nav).to have_content(s_(string_id))
end
end
end
end
context 'with a group selected' do
......
// Jest Snapshot v1, https://goo.gl/fbAQLP
exports[`PathNavigation matches snapshot 1`] = `
<div
background-color="#fafafa"
class="gl-path-nav"
>
<span
class="gl-path-fade gl-path-fade-left"
style="display: none;"
>
<button
class="gl-clear-icon-button"
>
<svg
class="gl-icon s32"
>
<use
href="#chevron-left"
/>
</svg>
</button>
</span>
<ul
class="gl-path-nav-list"
>
<li
class="gl-path-nav-list-item"
>
<button
class="gl-path-button gl-path-active-item-indigo"
>
<!---->
Issue
<!---->
</button>
</li>
<li
class="gl-path-nav-list-item"
>
<button
class="gl-path-button"
>
<!---->
Plan
<!---->
</button>
</li>
<li
class="gl-path-nav-list-item"
>
<button
class="gl-path-button"
>
<!---->
Code
<!---->
</button>
</li>
</ul>
<span
class="gl-path-fade gl-path-fade-right"
style="display: none;"
>
<button
class="gl-clear-icon-button"
>
<svg
class="gl-icon s32"
>
<use
href="#chevron-right"
/>
</svg>
</button>
</span>
</div>
`;
......@@ -8,6 +8,7 @@ import MockAdapter from 'axios-mock-adapter';
import GroupsDropdownFilter from 'ee/analytics/shared/components/groups_dropdown_filter.vue';
import ProjectsDropdownFilter from 'ee/analytics/shared/components/projects_dropdown_filter.vue';
import RecentActivityCard from 'ee/analytics/cycle_analytics/components/recent_activity_card.vue';
import PathNavigation from 'ee/analytics/cycle_analytics/components/path_navigation.vue';
import StageTable from 'ee/analytics/cycle_analytics/components/stage_table.vue';
import 'bootstrap';
import '~/gl_dropdown';
......@@ -48,6 +49,7 @@ function createComponent({
shallow = true,
withStageSelected = false,
scatterplotEnabled = true,
pathNavigationEnabled = false,
props = {},
} = {}) {
const func = shallow ? shallowMount : mount;
......@@ -66,6 +68,7 @@ function createComponent({
provide: {
glFeatures: {
cycleAnalyticsScatterplotEnabled: scatterplotEnabled,
valueStreamAnalyticsPathNavigation: pathNavigationEnabled,
},
},
...opts,
......@@ -132,6 +135,10 @@ describe('Cycle Analytics component', () => {
expect(wrapper.find(TypeOfWorkCharts).exists()).toBe(flag);
};
const displaysPathNavigation = flag => {
expect(wrapper.find(PathNavigation).exists()).toBe(flag);
};
beforeEach(() => {
mock = new MockAdapter(axios);
wrapper = createComponent();
......@@ -188,6 +195,10 @@ describe('Cycle Analytics component', () => {
expect(wrapper.find('.js-add-stage-button').exists()).toBe(false);
});
it('does not display the path navigation', () => {
displaysPathNavigation(false);
});
describe('hideGroupDropDown = true', () => {
beforeEach(() => {
mock = new MockAdapter(axios);
......@@ -254,6 +265,27 @@ describe('Cycle Analytics component', () => {
});
});
describe('path navigation', () => {
describe('disabled', () => {
it('does not display the path navigation', () => {
displaysPathNavigation(false);
});
});
describe('enabled', () => {
beforeEach(() => {
wrapper = createComponent({
withStageSelected: true,
pathNavigationEnabled: true,
});
});
it('displays the path navigation', () => {
displaysPathNavigation(true);
});
});
});
it('displays the duration chart', () => {
displaysDurationChart(true);
});
......@@ -299,7 +331,7 @@ describe('Cycle Analytics component', () => {
describe('the user does not have access to the group', () => {
beforeEach(() => {
mock = new MockAdapter(axios);
mock.onAny().reply(403);
mock.onAny().reply(httpStatusCodes.FORBIDDEN);
wrapper.vm.onGroupSelect(mockData.group);
return waitForPromises();
......@@ -339,6 +371,33 @@ describe('Cycle Analytics component', () => {
it('does not display the duration chart', () => {
displaysDurationChart(false);
});
describe('path navigation', () => {
describe('disabled', () => {
it('does not display the path navigation', () => {
displaysPathNavigation(false);
});
});
describe('enabled', () => {
beforeEach(() => {
wrapper = createComponent({
withStageSelected: true,
pathNavigationEnabled: true,
});
mock = new MockAdapter(axios);
mock.onAny().reply(httpStatusCodes.FORBIDDEN);
wrapper.vm.onGroupSelect(mockData.group);
return waitForPromises();
});
it('displays the path navigation', () => {
displaysPathNavigation(false);
});
});
});
});
});
});
......
import { GlPath, GlSkeletonLoading } from '@gitlab/ui';
import { mount } from '@vue/test-utils';
import Component from 'ee/analytics/cycle_analytics/components/path_navigation.vue';
import { transformedStagePathData, issueStage } from '../mock_data';
describe('PathNavigation', () => {
let wrapper = null;
const createComponent = props => {
return mount(Component, {
propsData: {
stages: transformedStagePathData,
selectedStage: issueStage,
loading: false,
...props,
},
});
};
const pathNavigationItems = () => {
return wrapper.findAll('.gl-path-button');
};
const clickItemAt = index => {
pathNavigationItems()
.at(index)
.trigger('click');
};
beforeEach(() => {
wrapper = createComponent();
});
afterEach(() => {
wrapper.destroy();
wrapper = null;
});
it('matches snapshot', () => {
expect(wrapper.element).toMatchSnapshot();
});
describe('displays correctly', () => {
it('has the correct props', () => {
expect(wrapper.find(GlPath).props('items')).toMatchObject(transformedStagePathData);
});
it('contains all the expected stages', () => {
const html = wrapper.find(GlPath).html();
transformedStagePathData.forEach(stage => {
expect(html).toContain(stage.title);
});
});
describe('loading', () => {
describe('is false', () => {
it('displays the gl-path component', () => {
expect(wrapper.find(GlPath).exists()).toBe(true);
});
it('hides the gl-skeleton-loading component', () => {
expect(wrapper.find(GlSkeletonLoading).exists()).toBe(false);
});
});
describe('is true', () => {
beforeEach(() => {
wrapper = createComponent({ loading: true });
});
it('hides the gl-path component', () => {
expect(wrapper.find(GlPath).exists()).toBe(false);
});
it('displays the gl-skeleton-loading component', () => {
expect(wrapper.find(GlSkeletonLoading).exists()).toBe(true);
});
});
});
});
describe('event handling', () => {
it('emits the selected event', () => {
expect(wrapper.emitted('selected')).toBeUndefined();
clickItemAt(0);
clickItemAt(1);
clickItemAt(2);
expect(wrapper.emittedByOrder()).toEqual([
{ name: 'selected', args: [transformedStagePathData[0]] },
{ name: 'selected', args: [transformedStagePathData[1]] },
{ name: 'selected', args: [transformedStagePathData[2]] },
]);
});
});
});
......@@ -13,6 +13,7 @@ import { toYmd } from 'ee/analytics/shared/utils';
import {
getTasksByTypeData,
transformRawTasksByTypeData,
transformStagesForPathNavigation,
} from 'ee/analytics/cycle_analytics/utils';
const fixtureEndpoints = {
......@@ -92,6 +93,15 @@ export const stageMedians = defaultStages.reduce((acc, stage) => {
};
}, {});
export const stageMediansWithNumericIds = defaultStages.reduce((acc, stage) => {
const { value } = getJSONFixture(fixtureEndpoints.stageMedian(stage));
const { id } = getStageByTitle(dummyState.stages, stage);
return {
...acc,
[id]: value,
};
}, {});
export const endDate = new Date(2019, 0, 14);
export const startDate = getDateInPast(endDate, DEFAULT_DAYS_IN_PAST);
......@@ -163,6 +173,12 @@ export const apiTasksByTypeData = getJSONFixture('analytics/type_of_work/tasks_b
export const rawTasksByTypeData = transformRawTasksByTypeData(apiTasksByTypeData);
export const transformedTasksByTypeData = getTasksByTypeData(apiTasksByTypeData);
export const transformedStagePathData = transformStagesForPathNavigation({
stages: allowedStages,
medians,
selectedStage: issueStage,
});
export const tasksByTypeData = {
seriesNames: ['Cool label', 'Normal label'],
data: [[0, 1, 2], [5, 2, 3], [2, 4, 1]],
......@@ -259,3 +275,6 @@ export const selectedProjects = [
avatarUrl: null,
},
];
// Value returned from JSON fixture is 345600 for issue stage which equals 4d
export const pathNavIssueMetric = '4d';
import * as getters from 'ee/analytics/cycle_analytics/store/getters';
import { startDate, endDate, allowedStages, selectedProjects } from '../mock_data';
import {
startDate,
endDate,
allowedStages,
selectedProjects,
transformedStagePathData,
issueStage,
stageMedians,
} from '../mock_data';
let state = null;
......@@ -158,4 +166,16 @@ describe('Cycle analytics getters', () => {
expect(resp).toEqual(result);
});
});
describe('pathNavigationData', () => {
it('returns the transformed data', () => {
state = {
stages: allowedStages,
medians: stageMedians,
selectedStage: issueStage,
};
expect(getters.pathNavigationData(state)).toEqual(transformedStagePathData);
});
});
});
......@@ -16,6 +16,7 @@ import {
flattenTaskByTypeSeries,
orderByDate,
toggleSelectedLabel,
transformStagesForPathNavigation,
} from 'ee/analytics/cycle_analytics/utils';
import { toYmd } from 'ee/analytics/shared/utils';
import {
......@@ -33,7 +34,12 @@ import {
issueStage,
rawCustomStage,
rawTasksByTypeData,
allowedStages,
stageMediansWithNumericIds,
totalStage,
pathNavIssueMetric,
} from './mock_data';
import { CAPITALIZED_STAGE_NAME, PATH_HOME_ICON } from 'ee/analytics/cycle_analytics/constants';
const labelEventIds = labelEvents.map(ev => ev.identifier);
......@@ -318,4 +324,48 @@ describe('Cycle analytics utils', () => {
expect(toggleSelectedLabel({ selectedLabelIds, value: 4 })).toEqual([1, 2, 3, 4]);
});
});
describe('transformStagesForPathNavigation', () => {
const stages = [...allowedStages, totalStage];
const response = transformStagesForPathNavigation({
stages,
medians: stageMediansWithNumericIds,
selectedStage: issueStage,
});
describe('transforms the data as expected', () => {
it('returns an array of stages', () => {
expect(Array.isArray(response)).toBe(true);
expect(response.length).toEqual(stages.length);
});
it('selects the correct stage', () => {
const selected = response.filter(stage => stage.selected === true)[0];
expect(selected.title).toEqual(issueStage.title);
});
it('includes the correct metric for the associated stage', () => {
const issue = response.filter(stage => stage.name === 'Issue')[0];
expect(issue.metric).toEqual(pathNavIssueMetric);
});
describe(`${CAPITALIZED_STAGE_NAME.OVERVIEW} stage specific changes`, () => {
const overview = response.filter(stage => stage.name === CAPITALIZED_STAGE_NAME.TOTAL)[0];
it(`renames '${CAPITALIZED_STAGE_NAME.TOTAL}' stage title to '${CAPITALIZED_STAGE_NAME.OVERVIEW}'`, () => {
expect(overview.title).toEqual(CAPITALIZED_STAGE_NAME.OVERVIEW);
});
it('includes the correct icon', () => {
expect(overview.icon).toEqual(PATH_HOME_ICON);
});
it(`moves the stage to the front`, () => {
expect(response[0]).toEqual(overview);
});
});
});
});
});
......@@ -23514,6 +23514,9 @@ msgstr ""
msgid "Value Stream Analytics gives an overview of how much time it takes to go from idea to production in your project."
msgstr ""
msgid "ValueStreamAnalytics|%{days}d"
msgstr ""
msgid "Variable"
msgstr ""
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment