diff --git a/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row.vue b/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row.vue index 20c5248052bc5d1f0601d27dcabb974356beb509..747e92b9e85e82a0e92e1e2fdc1b6c6b88577ff0 100644 --- a/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row.vue +++ b/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row.vue @@ -1,25 +1,11 @@ <script> -import { GlLink } from '@gitlab/ui'; - export default { name: 'CandidateDetailRow', - components: { - GlLink, - }, props: { label: { type: String, required: true, }, - text: { - type: [String, Number], - required: true, - }, - href: { - type: String, - required: false, - default: '', - }, sectionLabel: { type: String, required: false, @@ -34,8 +20,7 @@ export default { <td class="gl-text-secondary gl-font-weight-bold">{{ sectionLabel }}</td> <td class="gl-font-weight-bold">{{ label }}</td> <td> - <gl-link v-if="href" :href="href">{{ text }}</gl-link> - <template v-else>{{ text }}</template> + <slot></slot> </td> </tr> </template> diff --git a/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/ml_candidates_show.vue b/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/ml_candidates_show.vue index b7a612a9688b1efeaa5487ade184369922479c90..a68fb7d340a60a09061257b17d732d840e932790 100644 --- a/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/ml_candidates_show.vue +++ b/app/assets/javascripts/ml/experiment_tracking/routes/candidates/show/ml_candidates_show.vue @@ -1,4 +1,5 @@ <script> +import { GlAvatarLabeled, GlLink } from '@gitlab/ui'; import ModelExperimentsHeader from '~/ml/experiment_tracking/components/model_experiments_header.vue'; import DeleteButton from '~/ml/experiment_tracking/components/delete_button.vue'; import DetailRow from './components/candidate_detail_row.vue'; @@ -29,6 +30,8 @@ export default { ModelExperimentsHeader, DeleteButton, DetailRow, + GlAvatarLabeled, + GlLink, }, props: { candidate: { @@ -94,52 +97,51 @@ export default { <tbody> <tr class="divider"></tr> - <detail-row - :label="$options.i18n.ID_LABEL" - :section-label="$options.i18n.INFO_LABEL" - :text="info.iid" - /> + <detail-row :label="$options.i18n.ID_LABEL" :section-label="$options.i18n.INFO_LABEL"> + {{ info.iid }} + </detail-row> - <detail-row :label="$options.i18n.MLFLOW_ID_LABEL" :text="info.eid" /> + <detail-row :label="$options.i18n.MLFLOW_ID_LABEL">{{ info.eid }}</detail-row> - <detail-row :label="$options.i18n.STATUS_LABEL" :text="info.status" /> + <detail-row :label="$options.i18n.STATUS_LABEL">{{ info.status }}</detail-row> - <detail-row - :label="$options.i18n.EXPERIMENT_LABEL" - :text="info.experiment_name" - :href="info.path_to_experiment" - /> + <detail-row :label="$options.i18n.EXPERIMENT_LABEL"> + <gl-link :href="info.path_to_experiment"> + {{ info.experiment_name }} + </gl-link> + </detail-row> - <detail-row - v-if="info.path_to_artifact" - :label="$options.i18n.ARTIFACTS_LABEL" - :href="info.path_to_artifact" - :text="$options.i18n.ARTIFACTS_LABEL" - /> + <detail-row v-if="info.path_to_artifact" :label="$options.i18n.ARTIFACTS_LABEL"> + <gl-link :href="info.path_to_artifact"> + {{ $options.i18n.ARTIFACTS_LABEL }} + </gl-link> + </detail-row> <template v-if="ciJob"> <tr class="divider"></tr> <detail-row :label="$options.i18n.JOB_LABEL" - :text="ciJob.name" - :href="ciJob.path" :section-label="$options.i18n.CI_SECTION_LABEL" - /> + > + <gl-link :href="ciJob.path"> + {{ ciJob.name }} + </gl-link> + </detail-row> - <detail-row - v-if="ciJob.user" - :label="$options.i18n.CI_USER_LABEL" - :href="ciJob.user.path" - :text="ciJob.user.username" - /> + <detail-row v-if="ciJob.user" :label="$options.i18n.CI_USER_LABEL"> + <gl-avatar-labeled label="" :size="24" :src="ciJob.user.avatar"> + <gl-link :href="ciJob.user.path"> + {{ ciJob.user.name }} + </gl-link> + </gl-avatar-labeled> + </detail-row> - <detail-row - v-if="ciJob.merge_request" - :label="$options.i18n.CI_MR_LABEL" - :text="ciJob.merge_request.title" - :href="ciJob.merge_request.path" - /> + <detail-row v-if="ciJob.merge_request" :label="$options.i18n.CI_MR_LABEL"> + <gl-link :href="ciJob.merge_request.path"> + !{{ ciJob.merge_request.iid }} {{ ciJob.merge_request.title }} + </gl-link> + </detail-row> </template> <template v-for="{ sectionName, sectionValues } in sections"> @@ -150,8 +152,9 @@ export default { :key="item.name" :label="item.name" :section-label="index === 0 ? sectionName : ''" - :text="item.value" - /> + > + {{ item.value }} + </detail-row> </template> </tbody> </table> diff --git a/app/presenters/ml/candidate_details_presenter.rb b/app/presenters/ml/candidate_details_presenter.rb index 58ec2aee471064457f522c0e60b647f2513e2428..7f0bd9d6c110fd42f92c05c29112024d0efa1f3b 100644 --- a/app/presenters/ml/candidate_details_presenter.rb +++ b/app/presenters/ml/candidate_details_presenter.rb @@ -53,7 +53,9 @@ def user_info(user) { user: { path: user_path(user), - username: user.username + username: user.username, + name: user.name, + avatar: user.avatar_url } } end @@ -64,6 +66,7 @@ def mr_info(mr) { merge_request: { path: project_merge_request_path(mr.project, mr), + iid: mr.iid, title: mr.title } } diff --git a/spec/frontend/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row_spec.js b/spec/frontend/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row_spec.js index 8a39c5de2b340d57719ef96c50835ae10ea968ea..53dbd796d85142b858057bb923dac0ea597a6774 100644 --- a/spec/frontend/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row_spec.js +++ b/spec/frontend/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row_spec.js @@ -1,5 +1,4 @@ import { shallowMount } from '@vue/test-utils'; -import { GlLink } from '@gitlab/ui'; import DetailRow from '~/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row.vue'; describe('CandidateDetailRow', () => { @@ -9,14 +8,14 @@ describe('CandidateDetailRow', () => { let wrapper; - const createWrapper = (href = '') => { + const createWrapper = ({ slots = {} } = {}) => { wrapper = shallowMount(DetailRow, { - propsData: { sectionLabel: 'Section', label: 'Item', text: 'Text', href }, + propsData: { sectionLabel: 'Section', label: 'Item' }, + slots, }); }; const findCellAt = (index) => wrapper.findAll('td').at(index); - const findLink = () => findCellAt(ROW_VALUE_CELL).findComponent(GlLink); beforeEach(() => createWrapper()); @@ -28,22 +27,15 @@ describe('CandidateDetailRow', () => { expect(findCellAt(ROW_LABEL_CELL).text()).toBe('Item'); }); - describe('No href', () => { - it('Renders text', () => { - expect(findCellAt(ROW_VALUE_CELL).text()).toBe('Text'); - }); - - it('Does not render as link', () => { - expect(findLink().exists()).toBe(false); - }); + it('renders nothing on item cell', () => { + expect(findCellAt(ROW_VALUE_CELL).text()).toBe(''); }); - describe('With href', () => { - beforeEach(() => createWrapper('LINK')); + describe('With slot', () => { + beforeEach(() => createWrapper({ slots: { default: 'Some content' } })); - it('Renders link', () => { - expect(findLink().attributes().href).toBe('LINK'); - expect(findLink().text()).toBe('Text'); + it('Renders slot', () => { + expect(findCellAt(ROW_VALUE_CELL).text()).toBe('Some content'); }); }); }); diff --git a/spec/frontend/ml/experiment_tracking/routes/candidates/show/ml_candidates_show_spec.js b/spec/frontend/ml/experiment_tracking/routes/candidates/show/ml_candidates_show_spec.js index 07d501c4e44e9f3a819b5bbb2190d9c61042db23..0b3b780cb3fe358713476cf8ee8a1315380fc3a7 100644 --- a/spec/frontend/ml/experiment_tracking/routes/candidates/show/ml_candidates_show_spec.js +++ b/spec/frontend/ml/experiment_tracking/routes/candidates/show/ml_candidates_show_spec.js @@ -1,4 +1,5 @@ import { shallowMount } from '@vue/test-utils'; +import { GlAvatarLabeled, GlLink } from '@gitlab/ui'; import MlCandidatesShow from '~/ml/experiment_tracking/routes/candidates/show'; import DetailRow from '~/ml/experiment_tracking/routes/candidates/show/components/candidate_detail_row.vue'; import { TITLE_LABEL } from '~/ml/experiment_tracking/routes/candidates/show/translations'; @@ -9,6 +10,7 @@ import { newCandidate } from './mock_data'; describe('MlCandidatesShow', () => { let wrapper; const CANDIDATE = newCandidate(); + const USER_ROW = 6; const createWrapper = (createCandidate = () => CANDIDATE) => { wrapper = shallowMount(MlCandidatesShow, { @@ -19,8 +21,12 @@ describe('MlCandidatesShow', () => { const findDeleteButton = () => wrapper.findComponent(DeleteButton); const findHeader = () => wrapper.findComponent(ModelExperimentsHeader); const findNthDetailRow = (index) => wrapper.findAllComponents(DetailRow).at(index); + const findLinkInNthDetailRow = (index) => findNthDetailRow(index).findComponent(GlLink); const findSectionLabel = (label) => wrapper.find(`[sectionLabel='${label}']`); const findLabel = (label) => wrapper.find(`[label='${label}']`); + const findCiUserDetailRow = () => findNthDetailRow(USER_ROW); + const findCiUserAvatar = () => findCiUserDetailRow().findComponent(GlAvatarLabeled); + const findCiUserAvatarNameLink = () => findCiUserAvatar().findComponent(GlLink); describe('Header', () => { beforeEach(() => createWrapper()); @@ -42,36 +48,64 @@ describe('MlCandidatesShow', () => { describe('All info available', () => { beforeEach(() => createWrapper()); + const mrText = `!${CANDIDATE.info.ci_job.merge_request.iid} ${CANDIDATE.info.ci_job.merge_request.title}`; const expectedTable = [ - ['Info', 'ID', CANDIDATE.info.iid, ''], - ['', 'MLflow run ID', CANDIDATE.info.eid, ''], - ['', 'Status', CANDIDATE.info.status, ''], - ['', 'Experiment', CANDIDATE.info.experiment_name, CANDIDATE.info.path_to_experiment], - ['', 'Artifacts', 'Artifacts', CANDIDATE.info.path_to_artifact], - ['CI', 'Job', CANDIDATE.info.ci_job.name, CANDIDATE.info.ci_job.path], - ['', 'Triggered by', CANDIDATE.info.ci_job.user.username, CANDIDATE.info.ci_job.user.path], - [ - '', - 'Merge request', - CANDIDATE.info.ci_job.merge_request.title, - CANDIDATE.info.ci_job.merge_request.path, - ], - ['Parameters', CANDIDATE.params[0].name, CANDIDATE.params[0].value, ''], - ['', CANDIDATE.params[1].name, CANDIDATE.params[1].value, ''], - ['Metrics', CANDIDATE.metrics[0].name, CANDIDATE.metrics[0].value, ''], - ['', CANDIDATE.metrics[1].name, CANDIDATE.metrics[1].value, ''], - ['Metadata', CANDIDATE.metadata[0].name, CANDIDATE.metadata[0].value, ''], - ['', CANDIDATE.metadata[1].name, CANDIDATE.metadata[1].value, ''], + ['Info', 'ID', CANDIDATE.info.iid], + ['', 'MLflow run ID', CANDIDATE.info.eid], + ['', 'Status', CANDIDATE.info.status], + ['', 'Experiment', CANDIDATE.info.experiment_name], + ['', 'Artifacts', 'Artifacts'], + ['CI', 'Job', CANDIDATE.info.ci_job.name], + ['', 'Triggered by', 'CI User'], + ['', 'Merge request', mrText], + ['Parameters', CANDIDATE.params[0].name, CANDIDATE.params[0].value], + ['', CANDIDATE.params[1].name, CANDIDATE.params[1].value], + ['Metrics', CANDIDATE.metrics[0].name, CANDIDATE.metrics[0].value], + ['', CANDIDATE.metrics[1].name, CANDIDATE.metrics[1].value], + ['Metadata', CANDIDATE.metadata[0].name, CANDIDATE.metadata[0].value], + ['', CANDIDATE.metadata[1].name, CANDIDATE.metadata[1].value], ].map((row, index) => [index, ...row]); it.each(expectedTable)( 'row %s is created correctly', - (index, sectionLabel, label, text, href) => { - const row = findNthDetailRow(index); + (rowIndex, sectionLabel, label, text) => { + const row = findNthDetailRow(rowIndex); - expect(row.props()).toMatchObject({ sectionLabel, label, text, href }); + expect(row.props()).toMatchObject({ sectionLabel, label }); + expect(row.text()).toBe(text); }, ); + + describe('Table links', () => { + const linkRows = [ + [3, CANDIDATE.info.path_to_experiment], + [4, CANDIDATE.info.path_to_artifact], + [5, CANDIDATE.info.ci_job.path], + [7, CANDIDATE.info.ci_job.merge_request.path], + ]; + + it.each(linkRows)('row %s is created correctly', (rowIndex, href) => { + expect(findLinkInNthDetailRow(rowIndex).attributes().href).toBe(href); + }); + }); + + describe('CI triggerer', () => { + it('renders user row', () => { + const avatar = findCiUserAvatar(); + expect(avatar.props()).toMatchObject({ + label: '', + }); + expect(avatar.attributes().src).toEqual('/img.png'); + }); + + it('renders user name', () => { + const nameLink = findCiUserAvatarNameLink(); + + expect(nameLink.attributes().href).toEqual('path/to/ci/user'); + expect(nameLink.text()).toEqual('CI User'); + }); + }); + it('does not render params', () => { expect(findSectionLabel('Parameters').exists()).toBe(true); }); diff --git a/spec/frontend/ml/experiment_tracking/routes/candidates/show/mock_data.js b/spec/frontend/ml/experiment_tracking/routes/candidates/show/mock_data.js index 16c1b29f7bc477e8baed0332ad605d3decefc76b..3fbcf12299727370bf6ab6bfc315aeebcf0fb197 100644 --- a/spec/frontend/ml/experiment_tracking/routes/candidates/show/mock_data.js +++ b/spec/frontend/ml/experiment_tracking/routes/candidates/show/mock_data.js @@ -24,11 +24,14 @@ export const newCandidate = () => ({ path: 'path/to/job', merge_request: { path: 'path/to/mr', + iid: 1, title: 'Some MR', }, user: { path: 'path/to/ci/user', + name: 'CI User', username: 'ciuser', + avatar: '/img.png', }, }, }, diff --git a/spec/presenters/ml/candidate_details_presenter_spec.rb b/spec/presenters/ml/candidate_details_presenter_spec.rb index d83ffbc71299ca83db636d22227f464c94806944..9d1f6f634e4dfc21a85f8dd325204e6aa8065f1b 100644 --- a/spec/presenters/ml/candidate_details_presenter_spec.rb +++ b/spec/presenters/ml/candidate_details_presenter_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe ::Ml::CandidateDetailsPresenter, feature_category: :mlops do - let_it_be(:project) { create(:project, :private) } # rubocop:disable RSpec/FactoryBot/AvoidCreate - let_it_be(:user) { project.creator } + let_it_be(:user) { create(:user, :with_avatar) } # rubocop:disable RSpec/FactoryBot/AvoidCreate + let_it_be(:project) { create(:project, :private, creator: user) } # rubocop:disable RSpec/FactoryBot/AvoidCreate let_it_be(:experiment) { create(:ml_experiments, user: user, project: project) } # rubocop:disable RSpec/FactoryBot/AvoidCreate let_it_be(:candidate) do create(:ml_candidates, :with_artifact, experiment: experiment, user: user, project: project) # rubocop:disable RSpec/FactoryBot/AvoidCreate @@ -74,7 +74,9 @@ 'name' => 'test', 'user' => { 'path' => "/#{pipeline.user.username}", - 'username' => pipeline.user.username + 'name' => pipeline.user.name, + 'username' => pipeline.user.username, + 'avatar' => user.avatar_url } } @@ -100,6 +102,7 @@ it 'generates the correct ci' do expected_info = { 'path' => "/#{project.full_path}/-/merge_requests/#{mr.iid}", + 'iid' => mr.iid, 'title' => mr.title }