-
Notifications
You must be signed in to change notification settings - Fork 4
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
Changes from all commits
3d119a1
442336e
10b4dff
093d531
08e5d7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could try something like this but we can do it later |
||
) | ||
.filter((el) => el !== ''); | ||
} | ||
|
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 | ||
} | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 callthis.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.
There was a problem hiding this comment.
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.