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.

Sign up for GitHub

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

Fix/promptid selectors #48

Merged
merged 6 commits into from
Jan 12, 2024
Merged

Fix/promptid selectors #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
366117c
Fix for promptId related bugs
jthrilly Jan 11, 2024
4258004
small additional changes
jthrilly Jan 11, 2024
e501abd
Merge branch 'main' into fix/promptid-selectors
jthrilly Jan 11, 2024
ebf165c
small cleanup
jthrilly Jan 11, 2024
235e116
Merge branch 'fix/promptid-selectors' of https://github.com/complexda…
jthrilly Jan 11, 2024
b4c7a85
Merge branch 'main' into fix/promptid-selectors
jthrilly Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions lib/interviewer/selectors/__tests__/interface.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,6 @@ const mockState = {

describe('interface selector', () => {
describe('memoed selectors', () => {
it('makeGetIds()', () => {
const selected = Interface.makeGetIds();
expect(selected(mockState, mockProps)).toEqual({
stageId: mockStage.id,
promptId: mockPrompt.id,
});
});

Comment on lines -142 to -149
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this test because I also deleted the underlying selector. See below.

it('makeGetAdditionalAttributes()', () => {
const selected = Interface.makeGetAdditionalAttributes();
expect(selected(mockState, mockProps)).toEqual({ 'b6f2c4b9-e42f-459b-8f59-a11a685f460d': 2 });
Expand Down
18 changes: 11 additions & 7 deletions lib/interviewer/selectors/interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getSubjectType,
} from './prop';
import { createSelector } from '@reduxjs/toolkit';
import { getPromptIndex, getPrompts } from './session';

// Selectors that are generic between interfaces

Expand Down Expand Up @@ -130,11 +131,16 @@ export const makeGetStageNodeCount = () => {
* Return a filtered node list containing only nodes where node IDs contains the current promptId.
*/

export const getPromptId = createSelector(
getPrompts,
getPromptIndex,
(prompts, promptIndex) => prompts[promptIndex].id,
);

export const getNetworkNodesForPrompt = createSelector(
getNetworkNodesForType,
getPropPromptId,
(nodes, promptId) =>
filter(nodes, (node) => includes(node.promptIDs, promptId)),
getPromptId,
Copy link
Member Author

Choose a reason for hiding this comment

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

Replace the old selector with the new one - this selector was previously also failing without us realizing.

(nodes, promptId) => filter(nodes, (node) => includes(node.promptIDs, promptId)),
);

export const makeNetworkNodesForPrompt = () => getNetworkNodesForPrompt;
Expand All @@ -147,10 +153,8 @@ export const makeNetworkNodesForPrompt = () => getNetworkNodesForPrompt;
*/

export const getNetworkNodesForOtherPrompts = createSelector(
getNetworkNodesForType,
getPropPromptId,
(nodes, promptId) =>
filter(nodes, (node) => !includes(node.promptIDs, promptId)),
getNetworkNodesForType, getPromptId,
Copy link
Member Author

Choose a reason for hiding this comment

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

As above, update to use the session state selector rather than looking for prompt ID in props.

(nodes, promptId) => filter(nodes, (node) => !includes(node.promptIDs, promptId)),
);

export const makeNetworkNodesForOtherPrompts = () =>
Expand Down
27 changes: 21 additions & 6 deletions lib/interviewer/selectors/name-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { has } from 'lodash';
import { getProtocolCodebook } from './protocol';
import { getIds, getStageSubject, getSubjectType } from './prop';
import { getStageSubject, getSubjectType, propStage, propStageId } from './prop';
import { createSelector } from '@reduxjs/toolkit';
import { getPromptId } from './interface';

// Selectors that are specific to the name generator

Expand All @@ -23,13 +24,27 @@ const stageCardOptions = (_, props) => props.stage.cardOptions;
const stageSortOptions = (_, props) => props.stage.sortOptions;
const propPanels = (_, props) => props.stage.panels;

const getIDs = createSelector(
propStageId,
getPromptId,
(stageId, promptId) => {
return {
stageId,
promptId,
};
},
)

Comment on lines +27 to +37
Copy link
Member Author

@jthrilly jthrilly Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A "helper" selector that just gets the stage and prompt ID and returns them in an object.

This replaces the one I removed from the props selectors file, and puts it in a more appropriate location given that it now depends on state rather than props. I could also have put this in the interfaces selector file, and may do so if it turns out other interfaces use it.

This is in effect what fixes this issue.

export const getPromptModelData = createSelector(
getStageSubject,
getIds,
({ type }, ids) => ({
type,
...ids,
}),
getIDs,
({ type }, { stageId, promptId }) => {
return {
type,
stageId,
promptId,
};
},
Comment on lines +40 to +47
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual "fix" (although the real fix is in the new implementation of the getIDs selector, so as not to use props).

I also changed the style a little to remove the spread operator, which is something I want us to do more broadly. Spread operators can make it difficult to debug problems because it isn't clear which data they contain.

)

export const makeGetPromptNodeModelData = () => getPromptModelData;
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't understand the purpose of this function because it's not being called in anywhere

Expand Down
9 changes: 0 additions & 9 deletions lib/interviewer/selectors/prop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const getAdditionalAttributes = (stage, prompt) => {
export const propStage = (_, props) => props?.stage ?? null;
export const propPrompt = (_, props) => props?.prompt ?? null;
export const propStageId = (_, props) => props?.stage?.id ?? null;
export const getPropPromptId = (_, props) => props?.prompt?.id ?? null;
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this entirely because (1) it is broken now that prompt isn't consistently passed to props, and (2) I wanted to force myself to find other instances where this broken selector was used and fix them.


export const getStageOrPromptSubject = (stage, prompt) => {
return stage?.subject || prompt?.subject || null;
Expand All @@ -42,14 +41,6 @@ export const getSubjectType = createSelector(

export const makeGetSubject = () => getStageSubject;

// Returns current stage and prompt ID
export const getIds = createSelector(
propStageId, getPropPromptId,
(stageId, promptId) => ({ stageId, promptId }),
);
Comment on lines -45 to -49
Copy link
Member Author

Choose a reason for hiding this comment

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

This selector was replaced above, with a new implementation that uses session state.


export const makeGetIds = () => getIds;

export const getAdditionalAttributesSelector = createSelector(
propStage, propPrompt,
(stage, prompt) => getAdditionalAttributes(stage, prompt),
Expand Down
10 changes: 5 additions & 5 deletions lib/interviewer/selectors/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ export const getCurrentStage = createSelector(
},
);

export const getCaseId = createDeepEqualSelector(
getActiveSession,
(session) => session?.caseId,
);

export const getPromptIndex = createSelector(
getActiveSession,
(session) => session?.promptIndex ?? 0,
Expand All @@ -64,11 +69,6 @@ export const getCurrentPrompt = createSelector(
(stage, promptIndex) => stage?.prompts?.[promptIndex],
);

export const getCaseId = createDeepEqualSelector(
getActiveSession,
(session) => session?.caseId,
);

export const getPrompts = createSelector(
getCurrentStage,
(stage) => stage?.prompts,
Expand Down