From f25f3df70fa1a80d871dff9d46321a21fa8759a2 Mon Sep 17 00:00:00 2001 From: Chrystal Langston Date: Wed, 18 Aug 2021 09:21:32 -0400 Subject: [PATCH] Update RHS after creating outcome closes OUT-4687 flag=improved_outcomes_management test plan: NOTE: Make sure the Course you choose have less than 8 outcomes associated to the root group. Once the outcome is created, it will trigger a refetch and the outcome will be added to the selected group (in this case the root level group) and in the proper order. If the created outcome's group & title causes the outcome to be listed at the bottom of the list, you will need to use the scrolling feature to locate it. If you search for the outcome, the search causing its own refetch which is different from what is outlined in this PS. - Turn on FF - Navigation to a Course -> Outcomes - Do not click on a group from the LHS - Click on the Create button - Create an outcome by filling out the form and selecting the root group - Click save - Notice the outcome is not shown in the RHS. - Refresh the page. - Click on the root group from the LHS - Click on the Create button again - Create another outcome by filling out the form and selecting the root group - Click save - Notice that newly created outcome is in the list of outcomes in the RHS. - Add a few groups including nested groups. - Click on a parent group of a nested group. - Click on the create button and create an outcome. This time selecting a nested group of the parent. - Click save. - Notice the newly created outcome is in the list of outcomes in the RHS. - Conduct the same tests in an Account Change-Id: Ifbd746a2ba08990b8b30e853d3a439c18e6e031c Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271816 Tested-by: Service Cloud Jenkins Reviewed-by: Martin Yosifov QA-Review: Martin Yosifov Product-Review: Ben Friedman --- .../react/CreateOutcomeModal.js | 44 +++++++--- .../react/Management/__tests__/index.test.js | 43 ++++++++- .../react/Management/index.js | 15 +++- .../react/Management/shapes.js | 5 +- .../react/ManagementHeader.js | 10 ++- .../react/OutcomeManagement.js | 18 +++- .../__tests__/CreateOutcomeModal.test.js | 37 +++++++- .../react/__tests__/ManagementHeader.test.js | 1 + ui/shared/outcomes/mocks/Management.js | 87 ++++++++++++++++++- .../hooks/__tests__/useGroupDetail.test.js | 19 ++++ .../outcomes/react/hooks/useGroupDetail.js | 24 ++++- 11 files changed, 273 insertions(+), 30 deletions(-) diff --git a/ui/features/outcome_management/react/CreateOutcomeModal.js b/ui/features/outcome_management/react/CreateOutcomeModal.js index 23cb19de099..81833be460e 100644 --- a/ui/features/outcome_management/react/CreateOutcomeModal.js +++ b/ui/features/outcome_management/react/CreateOutcomeModal.js @@ -42,7 +42,7 @@ import useCanvasContext from '@canvas/outcomes/react/hooks/useCanvasContext' import {useMutation} from 'react-apollo' import OutcomesRceField from './shared/OutcomesRceField' -const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { +const CreateOutcomeModal = ({isOpen, onCloseHandler, onSuccess}) => { const {contextType, contextId, friendlyDescriptionFF, isMobileView} = useCanvasContext() const [title, titleChangeHandler] = useInput() const [displayName, displayNameChangeHandler] = useInput() @@ -51,14 +51,16 @@ const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { const [showTitleError, setShowTitleError] = useState(false) const [setOutcomeFriendlyDescription] = useMutation(SET_OUTCOME_FRIENDLY_DESCRIPTION_MUTATION) const [createLearningOutcome] = useMutation(CREATE_LEARNING_OUTCOME) - const {rootId, collections} = useManageOutcomes('OutcomeManagementPanel') - const [targetGroup, setTargetGroup] = useState(null) + const {rootId, collections, addNewGroup} = useManageOutcomes('OutcomeManagementPanel') + const [selectedGroup, setSelectedGroup] = useState(null) + const [selectedGroupAncestorIds, setSelectedGroupAncestorIds] = useState([]) useEffect(() => { - if (rootId && collections[rootId] && !targetGroup) { - setTargetGroup(collections[rootId]) + if (rootId && collections[rootId] && !selectedGroup) { + setSelectedGroup(collections[rootId]) + setSelectedGroupAncestorIds([rootId]) } - }, [collections, rootId, targetGroup]) + }, [collections, rootId, selectedGroup, selectedGroupAncestorIds]) const invalidTitle = titleValidator(title) const invalidDisplayName = displayNameValidator(displayName) @@ -75,17 +77,13 @@ const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { onCloseHandler() } - const handleSetTargetGroup = ({targetGroup}) => { - setTargetGroup(targetGroup) - } - const onCreateOutcomeHandler = () => { ;(async () => { try { const createLearningOutcomeResult = await createLearningOutcome({ variables: { input: { - groupId: targetGroup.id, + groupId: selectedGroup.id, title, displayName, description @@ -112,6 +110,11 @@ const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { } }) } + onSuccess({selectedGroupAncestorIds}) + // resetting selectedGroup to null otherwise it will be maintained + // and will cause the group to not be loaded in the GroupSelectedDrillDown + // when opening the create modal again + setSelectedGroup(null) showFlashAlert({ message: I18n.t('Outcome "%{title}" was successfully created.', {title}), @@ -206,7 +209,15 @@ const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { {isMobileView ? I18n.t('Select a location') : I18n.t('Location')} - + { + setSelectedGroupAncestorIds(targetAncestorsIds) + setSelectedGroup(targetGroup) + }} + onGroupCreated={addNewGroup} + modalName="CreateOutcomeModal" + /> @@ -218,7 +229,7 @@ const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { color="primary" margin="0 x-small 0 0" interaction={ - !invalidTitle && !invalidDisplayName && targetGroup ? 'enabled' : 'disabled' + !invalidTitle && !invalidDisplayName && selectedGroup ? 'enabled' : 'disabled' } onClick={onCreateOutcomeHandler} > @@ -230,9 +241,14 @@ const CreateOutcomeModal = ({isOpen, onCloseHandler}) => { ) } +CreateOutcomeModal.defaultProps = { + onSuccess: () => {} +} + CreateOutcomeModal.propTypes = { isOpen: PropTypes.bool.isRequired, - onCloseHandler: PropTypes.func.isRequired + onCloseHandler: PropTypes.func.isRequired, + onSuccess: PropTypes.func } export default CreateOutcomeModal diff --git a/ui/features/outcome_management/react/Management/__tests__/index.test.js b/ui/features/outcome_management/react/Management/__tests__/index.test.js index 41ad67cc8fd..d010381c596 100644 --- a/ui/features/outcome_management/react/Management/__tests__/index.test.js +++ b/ui/features/outcome_management/react/Management/__tests__/index.test.js @@ -87,10 +87,11 @@ describe('OutcomeManagementPanel', () => { contextType = 'Account', contextId = '1', canManage = true, - mocks = accountMocks({childGroupsCount: 0}) + mocks = accountMocks({childGroupsCount: 0}), + renderer = rtlRender } = {} ) => { - return rtlRender( + return renderer( { }) }) + describe('After an outcome is added', () => { + const createdOutcomeProps = (props = {}, ancestorIds = ['2']) => ({ + createdOutcomeGroupIds: ancestorIds, + ...props + }) + + it('it does not trigger a refetch if an outcome is created but not in the currently selected group', async () => { + const {getByText, queryByText, rerender} = render(, { + ...groupDetailDefaultProps + }) + await act(async () => jest.runOnlyPendingTimers()) + fireEvent.click(getByText('Course folder 0')) + await act(async () => jest.runOnlyPendingTimers()) + render(, { + ...groupDetailDefaultProps, + renderer: rerender + }) + await act(async () => jest.runOnlyPendingTimers()) + expect(queryByText(/Newly Created Outcome/)).not.toBeInTheDocument() + }) + + it('it does trigger a refetch if an outcome is created is in currently selected group in RHS', async () => { + const {getByText, queryByText, rerender} = render(, { + ...groupDetailDefaultProps + }) + await act(async () => jest.runOnlyPendingTimers()) + fireEvent.click(getByText('Course folder 0')) + await act(async () => jest.runOnlyPendingTimers()) + + render(, { + ...groupDetailDefaultProps, + renderer: rerender + }) + await act(async () => jest.runOnlyPendingTimers()) + expect(queryByText(/Newly Created Outcome/)).toBeInTheDocument() + }) + }) + describe('mobile', () => { beforeEach(() => { isMobileView = true diff --git a/ui/features/outcome_management/react/Management/index.js b/ui/features/outcome_management/react/Management/index.js index 337a36aed27..20f57c1501e 100644 --- a/ui/features/outcome_management/react/Management/index.js +++ b/ui/features/outcome_management/react/Management/index.js @@ -17,6 +17,7 @@ */ import React, {useCallback, useState, useEffect} from 'react' +import PropTypes from 'prop-types' import {Flex} from '@instructure/ui-flex' import {Spinner} from '@instructure/ui-spinner' import {Text} from '@instructure/ui-text' @@ -41,7 +42,7 @@ import OutcomeMoveModal from './OutcomeMoveModal' import ManageOutcomesBillboard from './ManageOutcomesBillboard' import GroupActionDrillDown from '../shared/GroupActionDrillDown' -const OutcomeManagementPanel = ({importNumber}) => { +const OutcomeManagementPanel = ({importNumber, createdOutcomeGroupIds}) => { const {isCourse, isMobileView, canManage} = useCanvasContext() const {setContainerRef, setLeftColumnRef, setDelimiterRef, setRightColumnRef, onKeyDownHandler} = useResize() @@ -75,7 +76,8 @@ const OutcomeManagementPanel = ({importNumber}) => { const {group, loading, loadMore, removeLearningOutcomes, readLearningOutcomes} = useGroupDetail({ id: selectedGroupId, - searchString: debouncedSearchString + searchString: debouncedSearchString, + rhsGroupIdsToRefetch: createdOutcomeGroupIds }) const selectedOutcomes = readLearningOutcomes(selectedOutcomeIds) @@ -421,4 +423,13 @@ const OutcomeManagementPanel = ({importNumber}) => { ) } +OutcomeManagementPanel.defaultProps = { + createdOutcomeGroupIds: [] +} + +OutcomeManagementPanel.propTypes = { + createdOutcomeGroupIds: PropTypes.arrayOf(PropTypes.string), + importNumber: PropTypes.number +} + export default OutcomeManagementPanel diff --git a/ui/features/outcome_management/react/Management/shapes.js b/ui/features/outcome_management/react/Management/shapes.js index 910fe816ba7..5ced2db91f5 100644 --- a/ui/features/outcome_management/react/Management/shapes.js +++ b/ui/features/outcome_management/react/Management/shapes.js @@ -45,8 +45,9 @@ export const groupCollectionShape = PropTypes.shape({ export const outcomeEdgesNodeShape = PropTypes.shape({ _id: PropTypes.string.isRequired, title: PropTypes.string.isRequired, - isImported: PropTypes.bool.isRequired, - description: PropTypes.string + isImported: PropTypes.bool, + description: PropTypes.string, + displayName: PropTypes.string }) export const outcomeEdgeShape = PropTypes.shape({ diff --git a/ui/features/outcome_management/react/ManagementHeader.js b/ui/features/outcome_management/react/ManagementHeader.js index e3ad360f774..a331968c01c 100644 --- a/ui/features/outcome_management/react/ManagementHeader.js +++ b/ui/features/outcome_management/react/ManagementHeader.js @@ -34,7 +34,7 @@ import CreateOutcomeModal from './CreateOutcomeModal' import useCanvasContext from '@canvas/outcomes/react/hooks/useCanvasContext' import useModal from '@canvas/outcomes/react/hooks/useModal' -const ManagementHeader = ({handleFileDrop, handleAddOutcomes}) => { +const ManagementHeader = ({handleFileDrop, handleAddOutcomes, onSuccessfulCreateOutcome}) => { const [isFindOutcomeModalOpen, openFindOutcomeModal, closeFindOutcomeModal] = useModal() const [isCreateOutcomeModalOpen, openCreateOutcomeModal, closeCreateOutcomeModal] = useModal() const {isMobileView, canManage, canImport} = useCanvasContext() @@ -112,14 +112,20 @@ const ManagementHeader = ({handleFileDrop, handleAddOutcomes}) => { ) } +ManagementHeader.defaultProps = { + onSuccessfulCreateOutcome: () => {} +} + ManagementHeader.propTypes = { handleFileDrop: PropTypes.func.isRequired, - handleAddOutcomes: PropTypes.func.isRequired + handleAddOutcomes: PropTypes.func.isRequired, + onSuccessfulCreateOutcome: PropTypes.func } export default ManagementHeader diff --git a/ui/features/outcome_management/react/OutcomeManagement.js b/ui/features/outcome_management/react/OutcomeManagement.js index a0cc0be8281..4360c2929d2 100644 --- a/ui/features/outcome_management/react/OutcomeManagement.js +++ b/ui/features/outcome_management/react/OutcomeManagement.js @@ -54,6 +54,7 @@ export const OutcomeManagementWithoutGraphql = ({breakpoints}) => { const [importRef, setImportRef] = useState(null) const [importNumber, setImportNumber] = useState(0) const [isImporting, setIsImporting] = useState(false) + const [createdOutcomeGroupIds, setCreatedOutcomeGroupIds] = useState([]) const [selectedIndex, setSelectedIndex] = useState(() => { const tabs = {'#mastery_scale': 1, '#mastery_calculation': 2} return window.location.hash in tabs ? tabs[window.location.hash] : 0 @@ -149,6 +150,10 @@ export const OutcomeManagementWithoutGraphql = ({breakpoints}) => { }) } + const onSuccessfulCreateOutcome = ({selectedGroupAncestorIds}) => { + setCreatedOutcomeGroupIds(selectedGroupAncestorIds) + } + const isMobileView = !breakpoints?.tablet const onAddOutcomes = addedOutcomes => { @@ -160,7 +165,11 @@ export const OutcomeManagementWithoutGraphql = ({breakpoints}) => { return ( {improvedManagement && ( - + )} { id="management" > {improvedManagement ? ( - !isImporting && + !isImporting && ( + + ) ) : ( )} diff --git a/ui/features/outcome_management/react/__tests__/CreateOutcomeModal.test.js b/ui/features/outcome_management/react/__tests__/CreateOutcomeModal.test.js index 48003d8b5e5..05847134a41 100644 --- a/ui/features/outcome_management/react/__tests__/CreateOutcomeModal.test.js +++ b/ui/features/outcome_management/react/__tests__/CreateOutcomeModal.test.js @@ -35,10 +35,12 @@ jest.useFakeTimers() describe('CreateOutcomeModal', () => { let onCloseHandlerMock + let onSuccessMock let cache const defaultProps = (props = {}) => ({ isOpen: true, onCloseHandler: onCloseHandlerMock, + onSuccess: onSuccessMock, ...props }) @@ -65,6 +67,7 @@ describe('CreateOutcomeModal', () => { beforeEach(() => { onCloseHandlerMock = jest.fn() + onSuccessMock = jest.fn() cache = createCache() }) @@ -142,7 +145,7 @@ describe('CreateOutcomeModal', () => { expect(getByText('Create').closest('button')).toHaveAttribute('disabled') }) - it('calls onCloseHandler on Create button click', async () => { + it('calls onCloseHandler & onSuccess on Create button click', async () => { const {getByLabelText, getByText} = render(, { mocks: [...smallOutcomeTree('Account')] }) @@ -173,6 +176,38 @@ describe('CreateOutcomeModal', () => { expect(within(getByRole('dialog')).getByText('Create').closest('button')).not.toBeDisabled() }) + it('calls onSuccess if create request succeeds', async () => { + const {getByText, getByLabelText} = render(, { + mocks: [ + ...smallOutcomeTree('Account'), + setFriendlyDescriptionOutcomeMock({ + inputDescription: 'Friendly Description value' + }), + createLearningOutcomeMock({ + title: 'Outcome 123', + displayName: 'Display name', + description: '', + groupId: '100' + }) + ] + }) + await act(async () => jest.runOnlyPendingTimers()) + fireEvent.change(getByLabelText('Name'), {target: {value: 'Outcome 123'}}) + fireEvent.change(getByLabelText('Friendly Name'), {target: {value: 'Display name'}}) + fireEvent.change(getByLabelText('Friendly description (for parent/student display)'), { + target: {value: 'Friendly Description value'} + }) + fireEvent.click(getByText('Account folder 0')) + fireEvent.click(getByText('Create')) + await act(async () => jest.runOnlyPendingTimers()) + await waitFor(() => { + expect(onSuccessMock).toHaveBeenCalledTimes(1) + expect(onSuccessMock).toHaveBeenCalledWith({ + selectedGroupAncestorIds: ['100', '1'] + }) + }) + }) + it('displays flash confirmation with proper message if create request succeeds', async () => { const showFlashAlertSpy = jest.spyOn(FlashAlert, 'showFlashAlert') const {getByText, getByLabelText} = render(, { diff --git a/ui/features/outcome_management/react/__tests__/ManagementHeader.test.js b/ui/features/outcome_management/react/__tests__/ManagementHeader.test.js index eedfe0efe59..2e0cbed0e9c 100644 --- a/ui/features/outcome_management/react/__tests__/ManagementHeader.test.js +++ b/ui/features/outcome_management/react/__tests__/ManagementHeader.test.js @@ -56,6 +56,7 @@ describe('ManagementHeader', () => { }) afterEach(() => { + showImportOutcomesModal.mockRestore() jest.clearAllMocks() }) diff --git a/ui/shared/outcomes/mocks/Management.js b/ui/shared/outcomes/mocks/Management.js index 45a5042029a..86be96063bd 100644 --- a/ui/shared/outcomes/mocks/Management.js +++ b/ui/shared/outcomes/mocks/Management.js @@ -519,7 +519,92 @@ export const groupDetailMocks = ({ __typename: 'LearningOutcomeGroup' } } - } + }, + // for testing graphqls refetch in index.js + newData: jest.fn(() => ({ + data: { + group: { + _id: groupId, + description: `${groupDescription} 4`, + title: `Refetched ${title}`, + outcomesCount: 3, + outcomes: { + pageInfo: { + hasNextPage: withMorePage, + endCursor: 'Mx', + __typename: 'PageInfo' + }, + edges: [ + { + canUnlink, + _id: '1', + node: { + _id: '1', + description: '', + title: `Refetched Outcome 1 - ${title}`, + displayName: '', + canEdit, + contextId, + contextType, + friendlyDescription: null, + __typename: 'LearningOutcome' + }, + group: { + _id: groupId, + title: `Refetched ${title}`, + __typename: 'LearningOutcomeGroup' + }, + __typename: 'ContentTag' + }, + { + canUnlink, + _id: '2', + node: { + _id: '2', + description: '', + title: `Refetched Outcome 2 - ${title}`, + displayName: '', + canEdit, + contextId, + contextType, + friendlyDescription: null, + __typename: 'LearningOutcome' + }, + group: { + _id: groupId, + title: `Refetched ${title}`, + __typename: 'LearningOutcomeGroup' + }, + __typename: 'ContentTag' + }, + { + canUnlink, + _id: '11', + node: { + _id: '11', + description: '', + title: `Newly Created Outcome - ${title}`, + displayName: '', + canEdit, + contextId, + contextType, + friendlyDescription: null, + __typename: 'LearningOutcome' + }, + group: { + _id: groupId, + title: `Refetched ${title}`, + __typename: 'LearningOutcomeGroup' + }, + __typename: 'ContentTag' + } + ], + __typename: 'ContentTagConnection' + }, + __typename: 'LearningOutcomeGroup' + } + } + })) }, { request: { diff --git a/ui/shared/outcomes/react/hooks/__tests__/useGroupDetail.test.js b/ui/shared/outcomes/react/hooks/__tests__/useGroupDetail.test.js index ab0d88201c5..d15ca898978 100644 --- a/ui/shared/outcomes/react/hooks/__tests__/useGroupDetail.test.js +++ b/ui/shared/outcomes/react/hooks/__tests__/useGroupDetail.test.js @@ -128,6 +128,25 @@ describe('groupDetailHook', () => { ]) }) + it('refetches when id is in rhsGroupIdsToRefetch', async () => { + mocks = [...groupDetailMocks(), ...groupDetailMocks({groupId: '200'})] + const {result, rerender} = renderHook( + id => useGroupDetail({id, rhsGroupIdsToRefetch: ['200']}), + {wrapper, initialProps: '1'} + ) + await act(async () => jest.runAllTimers()) + expect(result.current.group.title).toBe('Group 1') + expect(outcomeTitles(result)).toEqual(['Outcome 1 - Group 1', 'Outcome 2 - Group 1']) + act(() => rerender('200')) + await act(async () => jest.runAllTimers()) + expect(result.current.group.title).toBe('Refetched Group 200') + expect(outcomeTitles(result)).toEqual([ + 'Refetched Outcome 1 - Group 200', + 'Refetched Outcome 2 - Group 200', + 'Newly Created Outcome - Group 200' + ]) + }) + it('should not load group info if ACCOUNT_GROUP_ID passed as id', async () => { const {result} = renderHook(() => useGroupDetail({id: ACCOUNT_GROUP_ID}), {wrapper}) expect(result.current.loading).toBe(false) diff --git a/ui/shared/outcomes/react/hooks/useGroupDetail.js b/ui/shared/outcomes/react/hooks/useGroupDetail.js index 3da48c92062..c66cfca9131 100644 --- a/ui/shared/outcomes/react/hooks/useGroupDetail.js +++ b/ui/shared/outcomes/react/hooks/useGroupDetail.js @@ -22,7 +22,7 @@ import useCanvasContext from './useCanvasContext' import I18n from 'i18n!OutcomeManagement' import {showFlashAlert} from '@canvas/alerts/react/FlashAlert' import {SEARCH_GROUP_OUTCOMES} from '@canvas/outcomes/graphql/Management' -import {uniqWith, uniqBy, isEqual} from 'lodash' +import {uniqWith, uniqBy, uniq, isEqual} from 'lodash' import {gql} from '@canvas/apollo' const useAbortController = dependencies => { @@ -60,7 +60,8 @@ const useGroupDetail = ({ query = SEARCH_GROUP_OUTCOMES, loadOutcomesIsImported = false, searchString = '', - id + id, + rhsGroupIdsToRefetch = [] }) => { const {contextType, contextId, rootIds} = useCanvasContext() searchString = useSearchString(searchString) @@ -68,9 +69,14 @@ const useGroupDetail = ({ const queryVars = {outcomesContextType: contextType, outcomesContextId: contextId} const client = useApolloClient() const allVariables = useRef([]) + const refetchGroupIds = useRef([]) if (searchString) queryVars.searchQuery = searchString + useEffect(() => { + refetchGroupIds.current = uniq(refetchGroupIds.current.concat(rhsGroupIdsToRefetch)) + }, [rhsGroupIdsToRefetch]) + const skip = !id || rootIds.includes(id) const variables = { id, @@ -78,7 +84,7 @@ const useGroupDetail = ({ ...queryVars } - const {loading, error, data, fetchMore} = useQuery(query, { + const {loading, error, data, fetchMore, refetch} = useQuery(query, { variables, skip, context: { @@ -91,6 +97,15 @@ const useGroupDetail = ({ } }) + // To handle refetching of groups when an outcome is created. This will ensure that + // all groups including parent, grandparent, etc... are refetched. + useEffect(() => { + if (refetchGroupIds.current.includes(id)) { + refetchGroupIds.current = refetchGroupIds.current.filter(groupId => groupId !== id) + refetch() + } + }, [id, refetch]) + // this will handle the case when we click in group 1, wait for group 1 // load to finish, then click in a different group // it'll issue a query to load the second group @@ -224,7 +239,8 @@ const useGroupDetail = ({ error, loadMore, removeLearningOutcomes, - readLearningOutcomes + readLearningOutcomes, + refetch } }