-
Notifications
You must be signed in to change notification settings - Fork 25
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: set_item action: optionally target an index or id #2215
Merged
Merged
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
882cc4e
wip: set_item_at_index action
jfmcquade 33ac196
feat: add rest_data action
jfmcquade a406ad2
chore: clarified comment
jfmcquade 93d7102
Merge branch 'master' into feat/set-item-at-index
jfmcquade 11831a9
refactor: incorporate set_item_at_index logic into set_item instead
jfmcquade 63f4d7f
Merge branch 'master' into feat/set-item-at-index
jfmcquade 4107611
chore: remove workaround for fixed cdr issue; properly handle _index …
jfmcquade 60d5ef0
chore: handle _index === 0
jfmcquade 89885d5
Merge branch 'feat/data-items-eval-context' into feat/set-item-at-index
jfmcquade 40e86f9
fix: @item references in expressions within data-items loops
jfmcquade e1ea106
fix: complete merge of feat/data-items-eval-context
jfmcquade 8b3b134
chore: remove comments
jfmcquade afad693
Merge branch 'feat/data-items-eval-context' into feat/set-item-at-index
jfmcquade c3a014d
Merge branch 'feat/data-items-eval-context' into feat/set-item-at-index
jfmcquade 06cc641
chore: code tidy
jfmcquade 3ca52ca
chore: code tidy
jfmcquade c75bb43
Merge branch 'master' into feat/set-item-at-index
jfmcquade 989a715
chore: post-merge fix
jfmcquade 5108803
chore: tidy comments
jfmcquade f9f17ca
Merge branch 'master' into feat/set-item-at-index
jfmcquade 5c21fd8
chore: code tidy
jfmcquade fda82d6
fix: handle itemIndex passed from data items component
jfmcquade f96209d
chore: remove debug logs
jfmcquade 40c98cf
Merge branch 'master' into feat/set-item-at-index
esmeetewinkel ef3047a
chore: review code tidying
chrismclarke b8074b9
Merge branch 'master' into feat/set-item-at-index
esmeetewinkel e262c20
chore: tidy console logs
jfmcquade 350bd47
chore: reset theme file
jfmcquade 9f9f483
chore: tidy getTargetItemRow logic
jfmcquade 1b1e782
wip: create spec file for template variables service
jfmcquade 2e04f56
Merge branch 'refactor/extract-dynamic-fields' into feat/set-item-at-…
jfmcquade b315003
wip: template variable spec test setup
jfmcquade 72f89bb
test: add spec tests for template-variables service
jfmcquade 9ea2aeb
refactor: set_item logic
jfmcquade c327d3a
test: add spec tests for dynamic data setItem logic
jfmcquade 8a95b0d
refactor: set_item logic
jfmcquade 2a2f1e5
test: add spec tests for dynamic data setItem logic
jfmcquade daddda1
merge
jfmcquade a0120e3
Merge branch 'master' into feat/set-item-at-index
jfmcquade 378f256
chore: mock template-field and error-handler service bootstrap
chrismclarke 149c868
refactor: template-variable specs
chrismclarke 04168d3
chore: remove focused test
chrismclarke 74a3efb
Merge branch 'master' into feat/set-item-at-index
chrismclarke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 16 additions & 0 deletions
16
...s/template/services/template-calc.spec.ts → ...te/services/template-calc.service.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
181 changes: 181 additions & 0 deletions
181
src/app/shared/components/template/services/template-variables.service.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,181 @@ | ||
import { TestBed } from "@angular/core/testing"; | ||
import { IVariableContext, TemplateVariablesService } from "./template-variables.service"; | ||
import { HttpClientTestingModule } from "@angular/common/http/testing"; | ||
import { TemplateFieldService } from "./template-field.service"; | ||
import { MockTemplateFieldService } from "./template-field.service.spec"; | ||
import { AppDataService } from "src/app/shared/services/data/app-data.service"; | ||
import { CampaignService } from "src/app/feature/campaign/campaign.service"; | ||
import { MockAppDataService } from "src/app/shared/services/data/app-data.service.spec"; | ||
import { TemplateCalcService } from "./template-calc.service"; | ||
import { MockTemplateCalcService } from "./template-calc.service.spec"; | ||
|
||
const MOCK_APP_DATA = {}; | ||
|
||
// Fields populated to mock field service | ||
const MOCK_FIELDS = { | ||
_app_language: "gb_en", | ||
_app_skin: "default", | ||
string_field: "test_string_value", | ||
number_field: 2, | ||
}; | ||
|
||
const MOCK_CONTEXT_BASE: IVariableContext = { | ||
// Assume the row will have a dynamic 'field' entry | ||
field: "value", | ||
row: { | ||
type: "text", | ||
value: "", | ||
name: "test_row", | ||
_nested_name: "test_row", | ||
}, | ||
templateRowMap: {}, | ||
calcContext: { | ||
globalConstants: {}, | ||
globalFunctions: {}, | ||
thisCtxt: { | ||
fields: MOCK_FIELDS, | ||
local: {}, | ||
}, | ||
}, | ||
}; | ||
|
||
const TEST_FIELD_CONTEXT: IVariableContext = { | ||
...MOCK_CONTEXT_BASE, | ||
row: { | ||
...MOCK_CONTEXT_BASE.row, | ||
value: "Hello @fields.string_field", | ||
_dynamicFields: { | ||
value: [ | ||
{ | ||
fullExpression: "Hello @fields.string_field", | ||
matchedExpression: "@fields.string_field", | ||
type: "fields", | ||
fieldName: "string_field", | ||
}, | ||
], | ||
}, | ||
}, | ||
}; | ||
|
||
// Context adapted from this debug template: | ||
// https://docs.google.com/spreadsheets/d/1tL6CPHEIW-GPMYjdhVKQToy_hZ1H5qNIBkkh9XnA5QM/edit#gid=114708400 | ||
const TEST_ITEM_CONTEXT: IVariableContext = { | ||
...MOCK_CONTEXT_BASE, | ||
row: { | ||
...MOCK_CONTEXT_BASE.row, | ||
value: "@item._index + 1", | ||
// NOTE - any evaluated fields should appea | ||
_dynamicFields: { | ||
value: [ | ||
{ | ||
fullExpression: "@item._index + 1", | ||
matchedExpression: "@item._index", | ||
type: "item", | ||
fieldName: "_index", | ||
}, | ||
], | ||
}, | ||
}, | ||
itemContext: { | ||
id: "id1", | ||
number: 1, | ||
string: "hello", | ||
boolean: true, | ||
_index: 0, | ||
_id: "id1", | ||
_first: true, | ||
_last: false, | ||
}, | ||
}; | ||
|
||
/** | ||
* Call standalone tests via: | ||
* yarn ng test --include src/app/shared/components/template/services/template-variables.service.spec.ts | ||
*/ | ||
describe("TemplateVariablesService", () => { | ||
let service: TemplateVariablesService; | ||
let getNextCampaignRowsSpy: jasmine.Spy<jasmine.Func>; | ||
|
||
beforeEach(async () => { | ||
getNextCampaignRowsSpy = jasmine.createSpy(); | ||
TestBed.configureTestingModule({ | ||
imports: [HttpClientTestingModule], | ||
providers: [ | ||
{ | ||
provide: TemplateFieldService, | ||
useValue: new MockTemplateFieldService(MOCK_FIELDS), | ||
}, | ||
{ | ||
provide: AppDataService, | ||
useValue: new MockAppDataService(MOCK_APP_DATA), | ||
}, | ||
{ | ||
provide: TemplateCalcService, | ||
useValue: new MockTemplateCalcService(), | ||
}, | ||
// Mock single method from campaign service called | ||
{ | ||
provide: CampaignService, | ||
useValue: { | ||
ready: async () => { | ||
return true; | ||
}, | ||
getNextCampaignRows: getNextCampaignRowsSpy, | ||
}, | ||
}, | ||
], | ||
}); | ||
service = TestBed.inject(TemplateVariablesService); | ||
await service.ready(); | ||
}); | ||
|
||
it("should be created", () => { | ||
expect(service).toBeTruthy(); | ||
}); | ||
|
||
it("Evaluates PLH Data String", async () => { | ||
console.log({ TEST_FIELD_CONTEXT }); | ||
const res = await service.evaluatePLHData("Hello @fields.string_field", TEST_FIELD_CONTEXT); | ||
expect(res).toEqual("Hello test_string_value"); | ||
// Data will only be evaluated if it has been pre-parsed, extracting dynamic references | ||
// If not returns raw value | ||
delete TEST_FIELD_CONTEXT.row._dynamicFields; | ||
const resWithoutDynamicContext = await service.evaluatePLHData( | ||
"@fields.string_field", | ||
TEST_FIELD_CONTEXT | ||
); | ||
expect(resWithoutDynamicContext).toEqual("@fields.string_field"); | ||
/** | ||
* TODO - include all edge cases, e.g. raw, item, calc, deep-nested, object, array etc. | ||
const res = await service.evaluatePLHData(["@fields.string_field"], MOCK_CONTEXT); | ||
expect(res).toEqual({ 1: "test_string_value" }); | ||
const res = await service.evaluatePLHData( | ||
{ | ||
nested: "@fields.string_field", | ||
}, | ||
MOCK_CONTEXT | ||
); | ||
expect(res).toEqual({ nested: "test_string_value" }); | ||
*/ | ||
}); | ||
it("Evaluates condition strings", async () => { | ||
// Condition strings are evaluated without any previous pre-parsed dynamic fields | ||
const res = await service.evaluateConditionString("@fields.number_field > 3"); | ||
expect(res).toEqual(false); | ||
}); | ||
|
||
it("evaluates string containing item variable", async () => { | ||
const MOCK_ITEM_STRING = "@item._index + 1"; | ||
// Parse expression when item context included | ||
const resWithItemContext = await service.evaluatePLHData(MOCK_ITEM_STRING, TEST_ITEM_CONTEXT); | ||
expect(resWithItemContext).toEqual(1); | ||
// Retain raw expression if evaluating outside of item context | ||
// https://github.com/IDEMSInternational/parenting-app-ui/pull/2215#discussion_r1514757364 | ||
delete TEST_ITEM_CONTEXT.itemContext; | ||
const resWithoutItemContext = await service.evaluatePLHData( | ||
MOCK_ITEM_STRING, | ||
TEST_ITEM_CONTEXT | ||
); | ||
expect(resWithoutItemContext).toEqual(MOCK_ITEM_STRING); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This took me ages to figure out! There may be a nicer solution, but this is why something like this is needed:
Even though we have the check lower down (line 393), without this additional check then the string gets evaluated as JS, without the dynamic evaluator value being replaced. This meant that my string
@item._index + 1
was being evaluated as JS beforeitemContext
was available, evaluating to@item._index1
because it was treated as the addition of a string and a number,"@item._index" + 1
. Then, when the expression was re-evaluated after the data-items component had added theitemContext
,@item._index1
was evaluated with the value of@item_index
replaced. For the item at index0
, this actually meant the expression evaluated as01
, i.e.1
, and so for that one case the evaluation actually gave the expected value, which appeared as a false positive in my testing...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.
Really useful to catch, it would be good to include such things in spec test if possible