Commit 6cc8fe28 authored by Eulyeon Ko's avatar Eulyeon Ko Committed by Nicolò Maria Mezzopera

Resolve "Boards: reordering lists sending incorrect position" [RUN AS-IF-FOSS]

parent b8b6fb29
......@@ -79,7 +79,7 @@ export default {
'is-collapsed': list.collapsed,
'board-type-assignee': list.listType === 'assignee',
}"
:data-id="list.id"
:data-list-id="list.id"
class="board gl-display-inline-block gl-h-full gl-px-3 gl-vertical-align-top gl-white-space-normal is-expandable"
data-qa-selector="board_list"
>
......
......@@ -76,19 +76,6 @@ export default {
const el = this.canDragColumns ? this.$refs.list.$el : this.$refs.list;
el.scrollTo({ left: el.scrollWidth, behavior: 'smooth' });
},
handleDragOnEnd(params) {
const { item, newIndex, oldIndex, to } = params;
const listId = item.dataset.id;
const replacedListId = to.children[newIndex].dataset.id;
this.moveList({
listId,
replacedListId,
newIndex,
adjustmentValue: newIndex < oldIndex ? 1 : -1,
});
},
},
};
</script>
......@@ -104,7 +91,7 @@ export default {
ref="list"
v-bind="draggableOptions"
class="boards-list gl-w-full gl-py-5 gl-px-3 gl-white-space-nowrap"
@end="handleDragOnEnd"
@end="moveList"
>
<component
:is="boardColumnComponent"
......
......@@ -9,6 +9,7 @@ query ListIssues(
) {
group(fullPath: $fullPath) @include(if: $isGroup) {
board(id: $boardId) {
hideBacklogList
lists(issueFilters: $filters) {
nodes {
...BoardListFragment
......@@ -18,6 +19,7 @@ query ListIssues(
}
project(fullPath: $fullPath) @include(if: $isProject) {
board(id: $boardId) {
hideBacklogList
lists(issueFilters: $filters) {
nodes {
...BoardListFragment
......
import * as Sentry from '@sentry/browser';
import { sortBy } from 'lodash';
import {
BoardType,
ListType,
......@@ -216,33 +217,48 @@ export default {
},
moveList: (
{ state, commit, dispatch },
{ listId, replacedListId, newIndex, adjustmentValue },
{ state: { boardLists }, commit, dispatch },
{
item: {
dataset: { listId: movedListId },
},
newIndex,
to: { children },
},
) => {
if (listId === replacedListId) {
const displacedListId = children[newIndex].dataset.listId;
if (movedListId === displacedListId) {
return;
}
const { boardLists } = state;
const backupList = { ...boardLists };
const movedList = boardLists[listId];
const listIds = sortBy(
Object.keys(boardLists).filter(
(listId) =>
listId !== movedListId &&
boardLists[listId].listType !== ListType.backlog &&
boardLists[listId].listType !== ListType.closed,
),
(i) => boardLists[i].position,
);
const newPosition = newIndex - 1;
const listAtNewIndex = boardLists[replacedListId];
const targetPosition = boardLists[displacedListId].position;
// When the dragged list moves left, displaced list should shift right.
const shiftOffset = Number(boardLists[movedListId].position < targetPosition);
const displacedListIndex = listIds.findIndex((listId) => listId === displacedListId);
movedList.position = newPosition;
listAtNewIndex.position += adjustmentValue;
commit(types.MOVE_LIST, {
movedList,
listAtNewIndex,
});
dispatch('updateList', { listId, position: newPosition, backupList });
commit(
types.MOVE_LISTS,
listIds
.slice(0, displacedListIndex + shiftOffset)
.concat([movedListId], listIds.slice(displacedListIndex + shiftOffset))
.map((listId, index) => ({ listId, position: index })),
);
dispatch('updateList', { listId: movedListId, position: targetPosition });
},
updateList: (
{ commit, state: { issuableType, boardItemsByListId = {} }, dispatch },
{ listId, position, collapsed, backupList },
{ state: { issuableType, boardItemsByListId = {} }, dispatch },
{ listId, position, collapsed },
) => {
gqlClient
.mutate({
......@@ -255,8 +271,7 @@ export default {
})
.then(({ data }) => {
if (data?.updateBoardList?.errors.length) {
commit(types.UPDATE_LIST_FAILURE, backupList);
return;
throw new Error();
}
// Only fetch when board items havent been fetched on a collapsed list
......@@ -265,10 +280,19 @@ export default {
}
})
.catch(() => {
commit(types.UPDATE_LIST_FAILURE, backupList);
dispatch('handleUpdateListFailure');
});
},
handleUpdateListFailure: ({ dispatch, commit }) => {
dispatch('fetchLists');
commit(
types.SET_ERROR,
s__('Boards|An error occurred while updating the board list. Please try again.'),
);
},
toggleListCollapsed: ({ commit }, { listId, collapsed }) => {
commit(types.TOGGLE_LIST_COLLAPSED, { listId, collapsed });
},
......
......@@ -10,8 +10,7 @@ export const RECEIVE_BOARD_LISTS_SUCCESS = 'RECEIVE_BOARD_LISTS_SUCCESS';
export const RECEIVE_BOARD_LISTS_FAILURE = 'RECEIVE_BOARD_LISTS_FAILURE';
export const SHOW_PROMOTION_LIST = 'SHOW_PROMOTION_LIST';
export const RECEIVE_ADD_LIST_SUCCESS = 'RECEIVE_ADD_LIST_SUCCESS';
export const MOVE_LIST = 'MOVE_LIST';
export const UPDATE_LIST_FAILURE = 'UPDATE_LIST_FAILURE';
export const MOVE_LISTS = 'MOVE_LISTS';
export const TOGGLE_LIST_COLLAPSED = 'TOGGLE_LIST_COLLAPSED';
export const REMOVE_LIST = 'REMOVE_LIST';
export const REMOVE_LIST_FAILURE = 'REMOVE_LIST_FAILURE';
......
import { pull, union } from 'lodash';
import { cloneDeep, pull, union } from 'lodash';
import Vue from 'vue';
import { getIdFromGraphQLId } from '~/graphql_shared/utils';
import { s__ } from '~/locale';
......@@ -103,15 +103,12 @@ export default {
Vue.set(state.boardLists, list.id, list);
},
[mutationTypes.MOVE_LIST]: (state, { movedList, listAtNewIndex }) => {
const { boardLists } = state;
Vue.set(boardLists, movedList.id, movedList);
Vue.set(boardLists, listAtNewIndex.id, listAtNewIndex);
},
[mutationTypes.UPDATE_LIST_FAILURE]: (state, backupList) => {
state.error = s__('Boards|An error occurred while updating the list. Please try again.');
Vue.set(state, 'boardLists', backupList);
[mutationTypes.MOVE_LISTS]: (state, movedLists) => {
const updatedBoardList = movedLists.reduce((acc, { listId, position }) => {
acc[listId].position = position;
return acc;
}, cloneDeep(state.boardLists));
Vue.set(state, 'boardLists', updatedBoardList);
},
[mutationTypes.TOGGLE_LIST_COLLAPSED]: (state, { listId, collapsed }) => {
......
......@@ -148,18 +148,6 @@ export default {
'fetchItemsForList',
'doneLoadingSwimlanesItems',
]),
handleDragOnEnd(params) {
const { newIndex, oldIndex, item, to } = params;
const { listId } = item.dataset;
const replacedListId = to.children[newIndex].dataset.listId;
this.moveList({
listId,
replacedListId,
newIndex,
adjustmentValue: newIndex < oldIndex ? 1 : -1,
});
},
fetchMoreEpics() {
this.fetchEpicsSwimlanes({ fetchNext: true });
},
......@@ -215,7 +203,7 @@ export default {
v-bind="treeRootOptions"
class="board-swimlanes-headers gl-display-table gl-sticky gl-pt-5 gl-mb-5 gl-bg-white gl-top-0 gl-z-index-3"
data-testid="board-swimlanes-headers"
@end="handleDragOnEnd"
@end="moveList"
>
<div
v-for="list in lists"
......
......@@ -225,7 +225,7 @@ export default {
commit(types.SET_SHOW_LABELS, val);
},
updateListWipLimit({ commit, getters }, { maxIssueCount, listId }) {
updateListWipLimit({ commit, getters, dispatch }, { maxIssueCount, listId }) {
if (getters.shouldUseGraphQL) {
return gqlClient
.mutate({
......@@ -239,16 +239,17 @@ export default {
})
.then(({ data }) => {
if (data?.boardListUpdateLimitMetrics?.errors.length) {
commit(types.UPDATE_LIST_FAILURE);
} else {
const list = data.boardListUpdateLimitMetrics?.list;
throw new Error();
}
commit(types.UPDATE_LIST_SUCCESS, {
listId,
list,
list: data.boardListUpdateLimitMetrics?.list,
});
}
})
.catch(() => commit(types.UPDATE_LIST_FAILURE));
.catch(() => {
dispatch('handleUpdateListFailure');
});
}
return axios.put(`${boardsStoreEE.store.state.endpoints.listsEndpoint}/${listId}`, {
......
......@@ -17,10 +17,6 @@ export default {
Vue.set(state.boardLists, listId, list);
},
[mutationTypes.UPDATE_LIST_FAILURE]: (state) => {
state.error = s__('Boards|An error occurred while updating the list. Please try again.');
},
[mutationTypes.RECEIVE_ITEMS_FOR_LIST_SUCCESS]: (
state,
{ listItems, listPageInfo, listId, noEpicIssues },
......
......@@ -345,11 +345,11 @@ RSpec.describe 'Project issue boards', :js do
end
def list_weight_badge(list)
find(".board[data-id='gid://gitlab/List/#{list.id}'] [data-testid='issue-count-badge']")
find(".board[data-list-id='gid://gitlab/List/#{list.id}'] [data-testid='issue-count-badge']")
end
def card_weight_badge(list)
find(".board[data-id='gid://gitlab/List/#{list.id}'] [data-testid='board-card-weight']")
find(".board[data-list-id='gid://gitlab/List/#{list.id}'] [data-testid='board-card-weight']")
end
def visit_board_page
......
......@@ -328,7 +328,7 @@ RSpec.describe 'epic boards', :js do
end
def list_header(list)
find(".board[data-id='gid://gitlab/Boards::EpicList/#{list.id}'] .board-header")
find(".board[data-list-id='gid://gitlab/Boards::EpicList/#{list.id}'] .board-header")
end
def drag(selector: '.board-list', list_from_index: 0, from_index: 0, to_index: 0, list_to_index: 0, perform_drop: true)
......
......@@ -533,7 +533,7 @@ describe('updateListWipLimit', () => {
);
});
it('graphql - commit UPDATE_LIST_FAILURE mutation on failure', () => {
it('graphql - dispatch handleUpdateListFailure on failure', () => {
const maxIssueCount = 0;
const activeId = 1;
getters.shouldUseGraphQL = true;
......@@ -543,8 +543,8 @@ describe('updateListWipLimit', () => {
actions.updateListWipLimit,
{ maxIssueCount, listId: activeId },
{ isShowingEpicsSwimlanes: true, ...getters },
[{ type: types.UPDATE_LIST_FAILURE }],
[],
[{ type: 'handleUpdateListFailure' }],
);
});
});
......
......@@ -5371,7 +5371,7 @@ msgstr ""
msgid "Boards|An error occurred while removing the list. Please try again."
msgstr ""
msgid "Boards|An error occurred while updating the list. Please try again."
msgid "Boards|An error occurred while updating the board list. Please try again."
msgstr ""
msgid "Boards|Blocked by %{blockedByCount} %{issuableType}"
......
......@@ -70,13 +70,6 @@ RSpec.describe 'Project issue boards', :js do
stub_feature_flags(board_new_list: false)
visit_project_board_path_without_query_limit(project, board)
wait_for_requests
expect(page).to have_selector('.board', count: 4)
expect(find('.board:nth-child(2)')).to have_selector('.board-card')
expect(find('.board:nth-child(3)')).to have_selector('.board-card')
expect(find('.board:nth-child(4)')).to have_selector('.board-card')
end
it 'shows description tooltip on list title', :quarantine do
......@@ -221,18 +214,35 @@ RSpec.describe 'Project issue boards', :js do
it 'changes position of list' do
drag(list_from_index: 2, list_to_index: 1, selector: '.board-header')
wait_for_board_cards(2, 2)
wait_for_board_cards(3, 8)
wait_for_board_cards(4, 1)
expect(find('.board:nth-child(2) [data-testid="board-list-header"]')).to have_content(development.title)
expect(find('.board:nth-child(3) [data-testid="board-list-header"]')).to have_content(planning.title)
# Make sure list positions are preserved after a reload
visit_project_board_path_without_query_limit(project, board)
expect(find('.board:nth-child(2)')).to have_content(development.title)
expect(find('.board:nth-child(3)')).to have_content(planning.title)
expect(find('.board:nth-child(2) [data-testid="board-list-header"]')).to have_content(development.title)
expect(find('.board:nth-child(3) [data-testid="board-list-header"]')).to have_content(planning.title)
end
context 'without backlog and closed lists' do
let_it_be(:board) { create(:board, project: project, hide_backlog_list: true, hide_closed_list: true) }
let_it_be(:list1) { create(:list, board: board, label: planning, position: 0) }
let_it_be(:list2) { create(:list, board: board, label: development, position: 1) }
it 'changes position of list' do
visit_project_board_path_without_query_limit(project, board)
drag(list_from_index: 0, list_to_index: 1, selector: '.board-header')
expect(find('.board:nth-child(1) [data-testid="board-list-header"]')).to have_content(development.title)
expect(find('.board:nth-child(2) [data-testid="board-list-header"]')).to have_content(planning.title)
# Make sure list positions are preserved after a reload
visit_project_board_path_without_query_limit(project, board)
expect(find('.board:nth-child(2)')).to have_content(development.title)
expect(find('.board:nth-child(3)')).to have_content(planning.title)
expect(find('.board:nth-child(1) [data-testid="board-list-header"]')).to have_content(development.title)
expect(find('.board:nth-child(2) [data-testid="board-list-header"]')).to have_content(planning.title)
end
end
it 'dragging does not duplicate list' do
......@@ -682,6 +692,8 @@ RSpec.describe 'Project issue boards', :js do
def visit_project_board_path_without_query_limit(project, board)
inspect_requests(inject_headers: { 'X-GITLAB-DISABLE-SQL-QUERY-LIMIT' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/323426' }) do
visit project_board_path(project, board)
wait_for_requests
end
end
end
import * as Sentry from '@sentry/browser';
import { cloneDeep } from 'lodash';
import {
inactiveId,
ISSUABLE,
......@@ -419,75 +420,94 @@ describe('fetchLabels', () => {
});
describe('moveList', () => {
it('should commit MOVE_LIST mutation and dispatch updateList action', (done) => {
const initialBoardListsState = {
'gid://gitlab/List/1': mockLists[0],
'gid://gitlab/List/2': mockLists[1],
const backlogListId = 'gid://1';
const closedListId = 'gid://5';
const boardLists1 = {
'gid://3': { listType: '', position: 0 },
'gid://4': { listType: '', position: 1 },
'gid://5': { listType: '', position: 2 },
};
const state = {
fullPath: 'gitlab-org',
fullBoardId: 'gid://gitlab/Board/1',
boardType: 'group',
disabled: false,
boardLists: initialBoardListsState,
const boardLists2 = {
[backlogListId]: { listType: ListType.backlog, position: -Infinity },
[closedListId]: { listType: ListType.closed, position: Infinity },
...cloneDeep(boardLists1),
};
testAction(
actions.moveList,
{
listId: 'gid://gitlab/List/1',
replacedListId: 'gid://gitlab/List/2',
newIndex: 1,
adjustmentValue: 1,
const movableListsOrder = ['gid://3', 'gid://4', 'gid://5'];
const allListsOrder = [backlogListId, ...movableListsOrder, closedListId];
describe.each`
draggableFrom | draggableTo | boardLists | boardListsOrder | expectedMovableListsOrder
${0} | ${2} | ${boardLists1} | ${movableListsOrder} | ${['gid://4', 'gid://5', 'gid://3']}
${2} | ${0} | ${boardLists1} | ${movableListsOrder} | ${['gid://5', 'gid://3', 'gid://4']}
${0} | ${1} | ${boardLists1} | ${movableListsOrder} | ${['gid://4', 'gid://3', 'gid://5']}
${1} | ${2} | ${boardLists1} | ${movableListsOrder} | ${['gid://3', 'gid://5', 'gid://4']}
${2} | ${1} | ${boardLists1} | ${movableListsOrder} | ${['gid://3', 'gid://5', 'gid://4']}
${1} | ${3} | ${boardLists2} | ${allListsOrder} | ${['gid://4', 'gid://5', 'gid://3']}
${3} | ${1} | ${boardLists2} | ${allListsOrder} | ${['gid://5', 'gid://3', 'gid://4']}
${1} | ${2} | ${boardLists2} | ${allListsOrder} | ${['gid://4', 'gid://3', 'gid://5']}
${2} | ${3} | ${boardLists2} | ${allListsOrder} | ${['gid://3', 'gid://5', 'gid://4']}
${3} | ${2} | ${boardLists2} | ${allListsOrder} | ${['gid://3', 'gid://5', 'gid://4']}
`(
'when moving a list from position $draggableFrom to $draggableTo with lists $boardListsOrder',
({ draggableFrom, draggableTo, boardLists, boardListsOrder, expectedMovableListsOrder }) => {
const movedListId = boardListsOrder[draggableFrom];
const displacedListId = boardListsOrder[draggableTo];
const buildDraggablePayload = () => {
return {
item: { dataset: { listId: boardListsOrder[draggableFrom] } },
newIndex: draggableTo,
to: {
children: boardListsOrder.map((listId) => ({ dataset: { listId } })),
},
state,
[
};
};
it('should commit MOVE_LIST mutations and dispatch updateList action with correct payloads', () => {
return testAction({
action: actions.moveList,
payload: buildDraggablePayload(),
state: { boardLists },
expectedMutations: [
{
type: types.MOVE_LIST,
payload: { movedList: mockLists[0], listAtNewIndex: mockLists[1] },
type: types.MOVE_LISTS,
payload: expectedMovableListsOrder.map((listId, i) => ({ listId, position: i })),
},
],
[
expectedActions: [
{
type: 'updateList',
payload: {
listId: 'gid://gitlab/List/1',
position: 0,
backupList: initialBoardListsState,
listId: movedListId,
position: movableListsOrder.findIndex((i) => i === displacedListId),
},
},
],
done,
);
});
});
},
);
it('should not commit MOVE_LIST or dispatch updateList if listId and replacedListId are the same', () => {
const initialBoardListsState = {
'gid://gitlab/List/1': mockLists[0],
'gid://gitlab/List/2': mockLists[1],
};
const state = {
fullPath: 'gitlab-org',
fullBoardId: 'gid://gitlab/Board/1',
boardType: 'group',
disabled: false,
boardLists: initialBoardListsState,
};
describe('when moving from and to the same position', () => {
it('should not commit MOVE_LIST and should not dispatch updateList', () => {
const listId = 'gid://1000';
testAction(
actions.moveList,
{
listId: 'gid://gitlab/List/1',
replacedListId: 'gid://gitlab/List/1',
newIndex: 1,
adjustmentValue: 1,
return testAction({
action: actions.moveList,
payload: {
item: { dataset: { listId } },
newIndex: 0,
to: {
children: [{ dataset: { listId } }],
},
state,
[],
[],
);
},
state: { boardLists: { [listId]: { position: 0 } } },
expectedMutations: [],
expectedActions: [],
});
});
});
});
......@@ -549,7 +569,7 @@ describe('updateList', () => {
});
});
it('should commit UPDATE_LIST_FAILURE mutation when API returns an error', (done) => {
it('should dispatch handleUpdateListFailure when API returns an error', () => {
jest.spyOn(gqlClient, 'mutate').mockResolvedValue({
data: {
updateBoardList: {
......@@ -559,17 +579,31 @@ describe('updateList', () => {
},
});
testAction(
return testAction(
actions.updateList,
{ listId: 'gid://gitlab/List/1', position: 1 },
createState(),
[{ type: types.UPDATE_LIST_FAILURE }],
[],
done,
[{ type: 'handleUpdateListFailure' }],
);
});
});
describe('handleUpdateListFailure', () => {
it('should dispatch fetchLists action and commit SET_ERROR mutation', async () => {
await testAction({
action: actions.handleUpdateListFailure,
expectedMutations: [
{
type: types.SET_ERROR,
payload: 'An error occurred while updating the board list. Please try again.',
},
],
expectedActions: [{ type: 'fetchLists' }],
});
});
});
describe('toggleListCollapsed', () => {
it('should commit TOGGLE_LIST_COLLAPSED mutation', async () => {
const payload = { listId: 'gid://gitlab/List/1', collapsed: true };
......
......@@ -165,40 +165,26 @@ describe('Board Store Mutations', () => {
});
});
describe('MOVE_LIST', () => {
it('updates boardLists state with reordered lists', () => {
describe('MOVE_LISTS', () => {
it('updates the positions of board lists', () => {
state = {
...state,
boardLists: initialBoardListsState,
};
mutations.MOVE_LIST(state, {
movedList: mockLists[0],
listAtNewIndex: mockLists[1],
});
expect(state.boardLists).toEqual({
'gid://gitlab/List/2': mockLists[1],
'gid://gitlab/List/1': mockLists[0],
});
});
});
describe('UPDATE_LIST_FAILURE', () => {
it('updates boardLists state with previous order and sets error message', () => {
state = {
...state,
boardLists: {
'gid://gitlab/List/2': mockLists[1],
'gid://gitlab/List/1': mockLists[0],
mutations.MOVE_LISTS(state, [
{
listId: mockLists[0].id,
position: 1,
},
error: undefined,
};
mutations.UPDATE_LIST_FAILURE(state, initialBoardListsState);
{
listId: mockLists[1].id,
position: 0,
},
]);
expect(state.boardLists).toEqual(initialBoardListsState);
expect(state.error).toEqual('An error occurred while updating the list. Please try again.');
expect(state.boardLists[mockLists[0].id].position).toBe(1);
expect(state.boardLists[mockLists[1].id].position).toBe(0);
});
});
......
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