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.

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

feat(Feedback Rule): New myChoiceChosen token #1410

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { DynamicPrompt } from '../../../assets/wise5/directives/dynamic-prompt/D
styleUrls: ['./edit-dynamic-prompt.component.scss']
})
export class EditDynamicPromptComponent implements OnInit {
allowedReferenceComponentTypes: string[] = ['OpenResponse'];
protected allowedReferenceComponentTypes: string[] = ['MultipleChoice', 'OpenResponse'];
@Input() componentContent: any;
@Output() dynamicPromptChangedEvent = new EventEmitter<void>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { TeacherProjectService } from '../../../assets/wise5/services/teacherPro
styleUrls: ['./edit-question-bank.component.scss']
})
export class EditQuestionBankComponent implements OnInit {
allowedReferenceComponentTypes: string[] = ['OpenResponse'];
allowedReferenceComponentTypes: string[] = ['MultipleChoice', 'OpenResponse'];
@Input() componentContent: any;

constructor(private projectService: TeacherProjectService) {}
Expand Down
2 changes: 1 addition & 1 deletion src/app/services/studentPeerGroupService.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ function retrieveQuestionBankStudentData() {
describe('retrieveQuestionBankStudentData()', () => {
it('should retrieve question bank student data in preview', () => {
setIsPreview(true);
spyOn(projectService, 'getReferenceComponent').and.returnValue({
spyOn(projectService, 'getReferenceComponentForField').and.returnValue({
nodeId: nodeId1,
componentId: componentId1
});
Expand Down
11 changes: 4 additions & 7 deletions src/assets/wise5/components/common/cRater/CRaterResponse.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
import { Response } from '../feedbackRule/Response';
import { CRaterIdea } from './CRaterIdea';
import { CRaterScore } from './CRaterScore';

export class CRaterResponse {
export class CRaterResponse extends Response {
ideas: CRaterIdea[] = [];
score: number;
scores: CRaterScore[];
submitCounter: number;

constructor(jsonObject: any = {}) {
for (const key of Object.keys(jsonObject)) {
if (jsonObject[key] != null) {
this[key] = jsonObject[key];
}
}
super(jsonObject);
this.populateFields(jsonObject);
Copy link
Member

Choose a reason for hiding this comment

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

Is this call to populateFields() necessary if the parent also calls it?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we run the unit tests, because we initialize the ideas variable to an empty array ideas: CRaterIdea[] = []; in the class declaration, the webpack compiler (I think) adds this line below the line super(jsonObject) this.ideas = [];. If we don't call this.populateFields(jsonObject) again, the ideas fields is always an empty array, and causes the tests to fail. So in effect, this line is there just to make the tests pass, which is not ideal.

This problem goes away if we don't initialize the ideas variable to an empty array, but this may cause side-effects with the actual behavior of the program.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like somewhat unexpected behavior but is good to know for the future.

}

getDetectedIdeaCount(): number {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export class FeedbackRule {
}

static isSpecialRule(feedbackRule: FeedbackRule): boolean {
return ['isFinalSubmit', 'isSecondToLastSubmit', 'isNonScorable'].includes(
return ['isDefault', 'isFinalSubmit', 'isSecondToLastSubmit', 'isNonScorable'].includes(
feedbackRule.expression
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ let evaluator: FeedbackRuleEvaluator<CRaterResponse[]>;
describe('FeedbackRuleEvaluator', () => {
beforeEach(() => {
evaluator = new FeedbackRuleEvaluator(
new FeedbackRuleComponent(DEFAULT_FEEDBACK_RULES, 5, true)
new FeedbackRuleComponent(DEFAULT_FEEDBACK_RULES, 5, true),
null
);
});
matchRule_OneIdea();
Expand Down Expand Up @@ -76,7 +77,8 @@ function matchRule_hasKIScore() {
describe('hasKIScore()', () => {
beforeEach(() => {
evaluator = new FeedbackRuleEvaluator(
new FeedbackRuleComponent(HAS_KI_SCORE_FEEDBACK_RULES, 5, true)
new FeedbackRuleComponent(HAS_KI_SCORE_FEEDBACK_RULES, 5, true),
null
);
});
matchRule_hasKIScoreScoreInRange_ShouldMatchRule();
Expand Down Expand Up @@ -119,7 +121,10 @@ function matchRule_ideaCount() {
feedback: 'ideaCountLessThan(3)'
})
];
evaluator = new FeedbackRuleEvaluator(new FeedbackRuleComponent(feedbackRules, 5, true));
evaluator = new FeedbackRuleEvaluator(
new FeedbackRuleComponent(feedbackRules, 5, true),
null
);
});
matchRule_ideaCount_MatchRulesBasedOnNumIdeasFound();
});
Expand All @@ -142,7 +147,7 @@ function matchNoRule_ReturnDefault() {
function matchNoRule_NoDefaultFeedbackAuthored_ReturnApplicationDefault() {
it(`should return application default rule when no rule is matched and no default is
authored`, () => {
evaluator = new FeedbackRuleEvaluator(new FeedbackRuleComponent([], 5, true));
evaluator = new FeedbackRuleEvaluator(new FeedbackRuleComponent([], 5, true), null);
expectFeedback(['idea10', 'idea11'], [KI_SCORE_1], 1, evaluator.defaultFeedback);
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
import { Component } from '../../../common/Component';
import { ConstraintService } from '../../../services/constraintService';
import { FeedbackRuleComponent } from '../../feedbackRule/FeedbackRuleComponent';
import { CRaterResponse } from '../cRater/CRaterResponse';
import { FeedbackRule } from './FeedbackRule';
import { FeedbackRuleExpression } from './FeedbackRuleExpression';
import { Response } from './Response';
import { TermEvaluator } from './TermEvaluator/TermEvaluator';
import { TermEvaluatorFactory } from './TermEvaluator/TermEvaluatorFactory';

export class FeedbackRuleEvaluator<T extends CRaterResponse[]> {
export class FeedbackRuleEvaluator<T extends Response[]> {
defaultFeedback = $localize`Thanks for submitting your response.`;
protected factory = new TermEvaluatorFactory();
protected factory;
protected referenceComponent: Component;

constructor(protected component: FeedbackRuleComponent) {}
constructor(
protected component: FeedbackRuleComponent,
protected constraintService: ConstraintService
) {
this.factory = new TermEvaluatorFactory(constraintService);
}

getFeedbackRule(responses: T): FeedbackRule {
return (
Expand Down Expand Up @@ -38,7 +47,7 @@ export class FeedbackRuleEvaluator<T extends CRaterResponse[]> {
return (
this.hasMaxSubmitAndIsFinalSubmitRule(rule) &&
this.component.hasMaxSubmitCountAndUsedAllSubmits(
(responses[responses.length - 1] as CRaterResponse).submitCounter
responses[responses.length - 1].submitCounter
)
);
}
Expand All @@ -50,7 +59,7 @@ export class FeedbackRuleEvaluator<T extends CRaterResponse[]> {
protected satisfiesSecondToLastSubmitRule(responses: T, rule: FeedbackRule): boolean {
return (
this.hasMaxSubmitAndIsSecondToLastSubmitRule(rule) &&
this.isSecondToLastSubmit((responses[responses.length - 1] as CRaterResponse).submitCounter)
this.isSecondToLastSubmit(responses[responses.length - 1].submitCounter)
);
}

Expand Down Expand Up @@ -114,6 +123,7 @@ export class FeedbackRuleEvaluator<T extends CRaterResponse[]> {

protected evaluateTerm(term: string, responses: T): boolean {
const evaluator: TermEvaluator = this.factory.getTermEvaluator(term);
evaluator.setReferenceComponent(this.referenceComponent);
return TermEvaluator.requiresAllResponses(term)
? evaluator.evaluate(responses)
: evaluator.evaluate(responses[responses.length - 1]);
Expand All @@ -131,4 +141,8 @@ export class FeedbackRuleEvaluator<T extends CRaterResponse[]> {
})
);
}

setReferenceComponent(referenceComponent: Component): void {
this.referenceComponent = referenceComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ let evaluator: FeedbackRuleEvaluatorMultipleStudents;
describe('FeedbackRuleEvaluatorMultipleStudents', () => {
beforeEach(() => {
evaluator = new FeedbackRuleEvaluatorMultipleStudents(
new FeedbackRuleComponent(DEFAULT_FEEDBACK_RULES, 5, true)
new FeedbackRuleComponent(DEFAULT_FEEDBACK_RULES, 5, true),
null
);
});
matchRules_OneIdea();
Expand All @@ -38,7 +39,8 @@ function matchRules_HasKIScore() {
describe('hasKIScoreScore', () => {
beforeEach(() => {
evaluator = new FeedbackRuleEvaluatorMultipleStudents(
new FeedbackRuleComponent(HAS_KI_SCORE_FEEDBACK_RULES, 5, true)
new FeedbackRuleComponent(HAS_KI_SCORE_FEEDBACK_RULES, 5, true),
null
);
});
matchRules_hasKIScoreScoreInRange_ShouldMatchRule();
Expand Down Expand Up @@ -69,7 +71,10 @@ function matchNoRule_ReturnDefault() {
function matchNoRule_NoDefaultFeedbackAuthored_ReturnApplicationDefault() {
it(`should return application default rule when no rule is matched and no default is
authored`, () => {
evaluator = new FeedbackRuleEvaluatorMultipleStudents(new FeedbackRuleComponent([], 5, true));
evaluator = new FeedbackRuleEvaluatorMultipleStudents(
new FeedbackRuleComponent([], 5, true),
null
);
expectRules([createCRaterResponse(['idea10', 'idea11'], [KI_SCORE_1], 1)], ['default']);
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { CRaterResponse } from '../cRater/CRaterResponse';
import { FeedbackRule } from './FeedbackRule';
import { FeedbackRuleEvaluator } from './FeedbackRuleEvaluator';
import { Response } from './Response';
import { TermEvaluator } from './TermEvaluator/TermEvaluator';

export class FeedbackRuleEvaluatorMultipleStudents extends FeedbackRuleEvaluator<CRaterResponse[]> {
getFeedbackRules(responses: CRaterResponse[]): FeedbackRule[] {
export class FeedbackRuleEvaluatorMultipleStudents extends FeedbackRuleEvaluator<Response[]> {
getFeedbackRules(responses: Response[]): FeedbackRule[] {
const matchedRules = this.component
.getFeedbackRules()
.filter((rule) => this.satisfiesRule(responses, Object.assign(new FeedbackRule(), rule)));
Expand All @@ -13,10 +14,7 @@ export class FeedbackRuleEvaluatorMultipleStudents extends FeedbackRuleEvaluator
: [this.getDefaultRule(this.component.getFeedbackRules())];
}

protected satisfiesFinalSubmitRule(
responses: CRaterResponse[],
feedbackRule: FeedbackRule
): boolean {
protected satisfiesFinalSubmitRule(responses: Response[], feedbackRule: FeedbackRule): boolean {
return (
this.hasMaxSubmitAndIsFinalSubmitRule(feedbackRule) &&
responses.some((response: CRaterResponse) => {
Expand All @@ -26,12 +24,12 @@ export class FeedbackRuleEvaluatorMultipleStudents extends FeedbackRuleEvaluator
}

protected satisfiesSecondToLastSubmitRule(
responses: CRaterResponse[],
responses: Response[],
feedbackRule: FeedbackRule
): boolean {
return (
this.hasMaxSubmitAndIsSecondToLastSubmitRule(feedbackRule) &&
responses.some((response: CRaterResponse) => {
responses.some((response: Response) => {
return this.isSecondToLastSubmit(response.submitCounter);
})
);
Expand All @@ -49,9 +47,10 @@ export class FeedbackRuleEvaluatorMultipleStudents extends FeedbackRuleEvaluator
);
}

protected evaluateTerm(term: string, responses: CRaterResponse[]): boolean {
protected evaluateTerm(term: string, responses: Response[]): boolean {
const evaluator: TermEvaluator = this.factory.getTermEvaluator(term);
return responses.some((response: CRaterResponse) => {
evaluator.setReferenceComponent(this.referenceComponent);
return responses.some((response: Response) => {
return evaluator.evaluate(response);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class FeedbackRuleExpression {
return this.text
.replace(/ /g, '')
.split(
/(hasKIScore\(\d\)|accumulatedIdeaCountEquals\(\d\)|accumulatedIdeaCountLessThan\(\d\)|accumulatedIdeaCountMoreThan\(\d\)|ideaCountEquals\(\S+\)|ideaCountLessThan\(\S+\)|ideaCountMoreThan\(\S+\)|isSubmitNumber\(\d+\)|&&|\|\||!|\(|\))/g
/(hasKIScore\(\d\)|accumulatedIdeaCountEquals\(\d\)|accumulatedIdeaCountLessThan\(\d\)|accumulatedIdeaCountMoreThan\(\d\)|ideaCountEquals\(\S+\)|ideaCountLessThan\(\S+\)|ideaCountMoreThan\(\S+\)|myChoiceChosen\(\S+\)|isSubmitNumber\(\d+\)|&&|\|\||!|\(|\))/g
Copy link
Member

Choose a reason for hiding this comment

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

Can you split this up into multiple lines to make it more readable?

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 spent 30 minutes on this but couldn't figure it out. Can you? Or, in the interest of moving forward, let's save this for another time?

Copy link
Member

Choose a reason for hiding this comment

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

)
.filter((el) => el !== '');
}
Expand Down
14 changes: 14 additions & 0 deletions src/assets/wise5/components/common/feedbackRule/Response.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export class Response {
submitCounter: number;
constructor(jsonObject: any = {}) {
this.populateFields(jsonObject);
}

protected populateFields(jsonObject: any = {}): void {
for (const key of Object.keys(jsonObject)) {
if (jsonObject[key] != null) {
this[key] = jsonObject[key];
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { Component } from '../../../../common/Component';
import { ComponentContent } from '../../../../common/ComponentContent';
import { ConstraintService } from '../../../../services/constraintService';
import { CRaterResponse } from '../../cRater/CRaterResponse';
import { MyChoiceChosenTermEvaluator } from './MyChoiceChosenTermEvaluator';

class ConstraintServiceStub {
evaluateCriteria(criteria: any): boolean {
return true;
}
}

describe('MyChoiceChosenTermEvaluator', () => {
let evaluator1, mockConstraintService;
beforeEach(() => {
evaluator1 = new MyChoiceChosenTermEvaluator('myChoiceChosen("choice1")');
evaluator1.setReferenceComponent(
new Component({ id: 'componentA' } as ComponentContent, 'node1')
);
mockConstraintService = new ConstraintServiceStub();
evaluator1.setConstraintService(mockConstraintService as ConstraintService);
});
describe('evaluate()', () => {
[
{ description: 'choice is chosen', choiceChosen: true, expected: true },
{ description: 'choice is not chosen', choiceChosen: false, expected: false }
].forEach(({ description, choiceChosen, expected }) => {
describe(description, () => {
beforeEach(() => {
spyOn(mockConstraintService, 'evaluateCriteria').and.returnValue(choiceChosen);
});
it(`returns ${expected}`, () => {
expect(evaluator1.evaluate(new CRaterResponse())).toEqual(expected);
});
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { Response } from '../Response';
import { TermEvaluator } from './TermEvaluator';

export class MyChoiceChosenTermEvaluator extends TermEvaluator {
private choiceId: string;

constructor(term: string) {
super(term);
this.choiceId = term.match(/myChoiceChosen\("(\w+)"\)/)[1];
}

evaluate(response: Response | Response[]): boolean {
return this.constraintService.evaluateCriteria({
name: 'choiceChosen',
params: {
nodeId: this.referenceComponent.nodeId,
componentId: this.referenceComponent.id,
choiceIds: this.choiceId
}
});
}
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
import { CRaterResponse } from '../../cRater/CRaterResponse';
import { Component } from '../../../../common/Component';
import { ConstraintService } from '../../../../services/constraintService';
import { Response } from '../Response';

export abstract class TermEvaluator {
protected referenceComponent: Component;
protected constraintService: ConstraintService;

constructor(protected term: string) {}
abstract evaluate(response: CRaterResponse | CRaterResponse[]): boolean;
abstract evaluate(response: Response | Response[]): boolean;

static isAccumulatedIdeaCountTerm(term: string): boolean {
return /accumulatedIdeaCount(MoreThan|Equals|LessThan)\([\d+]\)/.test(term);
}

static isMyChoiceChosenTerm(term: string): boolean {
return /myChoiceChosen\("\w+"\)/.test(term);
}

static isHasKIScoreTerm(term: string): boolean {
return /hasKIScore\([1-5]\)/.test(term);
}
Expand All @@ -30,4 +39,12 @@ export abstract class TermEvaluator {
TermEvaluator.isIdeaCountWithResponseIndexTerm(term)
);
}

setConstraintService(service: ConstraintService): void {
this.constraintService = service;
}

setReferenceComponent(referenceComponent: Component): void {
this.referenceComponent = referenceComponent;
}
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
import { MyChoiceChosenTermEvaluator } from './MyChoiceChosenTermEvaluator';
import { HasKIScoreTermEvaluator } from './HasKIScoreTermEvaluator';
import { IdeaCountTermEvaluator } from './IdeaCountTermEvaluator';
import { IdeaTermEvaluator } from './IdeaTermEvaluator';
import { IsSubmitNumberEvaluator } from './IsSubmitNumberEvaluator';
import { TermEvaluatorFactory } from './TermEvaluatorFactory';

describe('TermEvaluatorFactory', () => {
const factory = new TermEvaluatorFactory();
const factory = new TermEvaluatorFactory(null);
describe('getTermEvaluator()', () => {
it('should return correct evaluator', () => {
[
{
term: 'myChoiceChosen("choice1")',
instanceType: MyChoiceChosenTermEvaluator
},
{ term: 'hasKIScore(3)', instanceType: HasKIScoreTermEvaluator },
{ term: 'ideaCountMoreThan(1)', instanceType: IdeaCountTermEvaluator },
{ term: 'ideaCountEquals(3)', instanceType: IdeaCountTermEvaluator },
Expand Down
Loading