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: set_item action: optionally target an index or id #2215

Merged
merged 43 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
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 Mar 1, 2024
33ac196
feat: add rest_data action
jfmcquade Mar 1, 2024
a406ad2
chore: clarified comment
jfmcquade Mar 1, 2024
93d7102
Merge branch 'master' into feat/set-item-at-index
jfmcquade Mar 5, 2024
11831a9
refactor: incorporate set_item_at_index logic into set_item instead
jfmcquade Mar 5, 2024
63f4d7f
Merge branch 'master' into feat/set-item-at-index
jfmcquade Mar 6, 2024
4107611
chore: remove workaround for fixed cdr issue; properly handle _index …
jfmcquade Mar 6, 2024
60d5ef0
chore: handle _index === 0
jfmcquade Mar 6, 2024
89885d5
Merge branch 'feat/data-items-eval-context' into feat/set-item-at-index
jfmcquade Mar 6, 2024
40e86f9
fix: @item references in expressions within data-items loops
jfmcquade Mar 6, 2024
e1ea106
fix: complete merge of feat/data-items-eval-context
jfmcquade Mar 6, 2024
8b3b134
chore: remove comments
jfmcquade Mar 6, 2024
afad693
Merge branch 'feat/data-items-eval-context' into feat/set-item-at-index
jfmcquade Mar 6, 2024
c3a014d
Merge branch 'feat/data-items-eval-context' into feat/set-item-at-index
jfmcquade Mar 6, 2024
06cc641
chore: code tidy
jfmcquade Mar 6, 2024
3ca52ca
chore: code tidy
jfmcquade Mar 6, 2024
c75bb43
Merge branch 'master' into feat/set-item-at-index
jfmcquade Apr 3, 2024
989a715
chore: post-merge fix
jfmcquade Apr 3, 2024
5108803
chore: tidy comments
jfmcquade Apr 4, 2024
f9f17ca
Merge branch 'master' into feat/set-item-at-index
jfmcquade Apr 5, 2024
5c21fd8
chore: code tidy
jfmcquade Apr 5, 2024
fda82d6
fix: handle itemIndex passed from data items component
jfmcquade Apr 5, 2024
f96209d
chore: remove debug logs
jfmcquade Apr 5, 2024
40c98cf
Merge branch 'master' into feat/set-item-at-index
esmeetewinkel Apr 12, 2024
ef3047a
chore: review code tidying
chrismclarke Apr 12, 2024
b8074b9
Merge branch 'master' into feat/set-item-at-index
esmeetewinkel Apr 19, 2024
e262c20
chore: tidy console logs
jfmcquade Apr 22, 2024
350bd47
chore: reset theme file
jfmcquade Apr 22, 2024
9f9f483
chore: tidy getTargetItemRow logic
jfmcquade Apr 22, 2024
1b1e782
wip: create spec file for template variables service
jfmcquade Apr 22, 2024
2e04f56
Merge branch 'refactor/extract-dynamic-fields' into feat/set-item-at-…
jfmcquade Apr 22, 2024
b315003
wip: template variable spec test setup
jfmcquade Apr 23, 2024
72f89bb
test: add spec tests for template-variables service
jfmcquade Apr 24, 2024
9ea2aeb
refactor: set_item logic
jfmcquade Apr 24, 2024
c327d3a
test: add spec tests for dynamic data setItem logic
jfmcquade Apr 24, 2024
8a95b0d
refactor: set_item logic
jfmcquade Apr 24, 2024
2a2f1e5
test: add spec tests for dynamic data setItem logic
jfmcquade Apr 24, 2024
daddda1
merge
jfmcquade Apr 24, 2024
a0120e3
Merge branch 'master' into feat/set-item-at-index
jfmcquade Apr 24, 2024
378f256
chore: mock template-field and error-handler service bootstrap
chrismclarke Apr 29, 2024
149c868
refactor: template-variable specs
chrismclarke Apr 30, 2024
04168d3
chore: remove focused test
chrismclarke Apr 30, 2024
74a3efb
Merge branch 'master' into feat/set-item-at-index
chrismclarke Apr 30, 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
13 changes: 10 additions & 3 deletions packages/data-models/flowTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,17 +340,24 @@ export namespace FlowTypes {
_evalContext?: { itemContext: TemplateRowItemEvalContext }; // force specific context variables when calculating eval statements (such as loop items)
__EMPTY?: any; // empty cells (can be removed after pr 679 merged)
}

export type IDynamicField = { [key: string]: IDynamicField | TemplateRowDynamicEvaluator[] };

export interface TemplateRowItemEvalContext {
export interface TemplateRowItemEvalContextMetadata {
// item metadata
_id: string;
_index: number;
_first: boolean;
_last: boolean;
// item data
[key: string]: any;
}
// Enumerable list of metadata columns for use by processing functions
export const TEMPLATE_ROW_ITEM_METADATA_FIELDS: Array<keyof TemplateRowItemEvalContextMetadata> =
["_id", "_index", "_first", "_last"];

// General interface for row items which can contain any key-value pairs with metadata
export type TemplateRowItemEvalContext = TemplateRowItemEvalContextMetadata & {
[key: string]: any;
};

type IDynamicPrefix = IAppConfig["DYNAMIC_PREFIXES"][number];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import {
OnDestroy,
} from "@angular/core";
import { debounceTime, Subscription } from "rxjs";
import { DynamicDataService } from "src/app/shared/services/dynamic-data/dynamic-data.service";
import {
DynamicDataService,
ISetItemContext,
} from "src/app/shared/services/dynamic-data/dynamic-data.service";
import { FlowTypes } from "../../models";
import { ItemProcessor } from "../../processors/item";
import { TemplateRowService } from "../../services/instance/template-row.service";
Expand Down Expand Up @@ -96,8 +99,9 @@ export class TmplDataItemsComponent extends TemplateBaseComponent implements OnD
const itemDataIDs = itemData.map((item) => item.id);
// Reassign metadata fields previously assigned by item as rendered row count may have changed
return templateRows.map((r) => {
const itemId = r._evalContext.itemContext._id;
// Map the row item context to the original list of items rendered to know position in item list.
const itemIndex = itemDataIDs.indexOf(r._evalContext.itemContext._id);
const itemIndex = itemDataIDs.indexOf(itemId);
// Update metadata fields as _first, _last and index may have changed based on dynamic updates
r._evalContext.itemContext = {
...r._evalContext.itemContext,
Expand All @@ -108,14 +112,19 @@ export class TmplDataItemsComponent extends TemplateBaseComponent implements OnD
// Update any action list set_item args to contain name of current data list and item id
// and set_items action to include all currently displayed rows
if (r.action_list) {
const setItemContext: ISetItemContext = {
flow_name: this.dataListName,
itemDataIDs,
currentItemId: itemId,
};
r.action_list = r.action_list.map((a) => {
if (a.action_id === "set_item") {
a.args = [this.dataListName, r._evalContext.itemContext._id];
a.args = [setItemContext];
}
if (a.action_id === "set_items") {
// TODO - add a check for @item refs and replace parameter list with correct values
// for each individual item (default will be just to pick the first)
a.args = [this.dataListName, itemDataIDs];
a.args = [setItemContext];
}
return a;
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
import { ICalcContext, TemplateCalcService } from "./template-calc.service";

export class MockTemplateCalcService implements Partial<TemplateCalcService> {
public async ready(): Promise<boolean> {
return true;
}

public getCalcContext(): ICalcContext {
return {
thisCtxt: {},
globalConstants: {},
globalFunctions: {},
};
}
}

/**
* TODO - Add testing data and methods
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { TestBed } from "@angular/core/testing";
import { TemplateFieldService } from "./template-field.service";
import type { PromiseExtended } from "dexie";
import { booleanStringToBoolean } from "src/app/shared/utils";
import { ErrorHandlerService } from "src/app/shared/services/error-handler/error-handler.service";
import { MockErrorHandlerService } from "src/app/shared/services/error-handler/error-handler.service.spec";

/** Mock calls for field values from the template field service to return test data */
export class MockTemplateFieldService implements Partial<TemplateFieldService> {
Expand All @@ -27,11 +29,13 @@ export class MockTemplateFieldService implements Partial<TemplateFieldService> {
describe("TemplateFieldService", () => {
let service: TemplateFieldService;

beforeEach(() => {
beforeEach(async () => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [{ provide: ErrorHandlerService, useValue: new MockErrorHandlerService() }],
});
service = TestBed.inject(TemplateFieldService);
await service.ready();
});

it("should be created", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export class MockTemplateTranslateService implements Partial<TemplateTranslateSe
}
}

describe("TaskService", () => {
describe("TemplateTranslateService", () => {
let service: TemplateTranslateService;

beforeEach(() => {
Expand Down
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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const log = SHOW_DEBUG_LOGS ? console.log : () => null;
const log_group = SHOW_DEBUG_LOGS ? console.group : () => null;
const log_groupEnd = SHOW_DEBUG_LOGS ? console.groupEnd : () => null;

const { TEMPLATE_ROW_ITEM_METADATA_FIELDS } = FlowTypes;

/**
* Most methods in this class depend on factors relating to the execution context
* (e.g.row, variables etc.). Store as a single object to make it easier to pass between methods
Expand Down Expand Up @@ -120,12 +122,19 @@ export class TemplateVariablesService extends AsyncServiceBase {
}

/**
* Inore evaluation of meta, comment, and specifiedfields.
* Ignore evaluation of meta, comment, and specifiedfields.
* Could provide single list of approved fields, but as dynamic fields also can be found in parameter lists
* would likely prove too restrictive
**/
private shouldEvaluateField(fieldName: keyof FlowTypes.TemplateRow, omitFields: string[] = []) {
private shouldEvaluateField(
fieldName: keyof FlowTypes.TemplateRow | keyof FlowTypes.TemplateRowItemEvalContextMetadata,
omitFields: string[] = []
) {
if (omitFields.includes(fieldName)) return false;

// Evaluate fields that are names of item metadata fields, e.g. "_index", "_id",
// E.g. for use in actions such as `click | set_item | _index: @item._index + 1, completed:false`
if (TEMPLATE_ROW_ITEM_METADATA_FIELDS.includes(fieldName as any)) return true;
if (fieldName.startsWith("_")) return false;
return true;
}
Expand Down Expand Up @@ -183,6 +192,11 @@ export class TemplateVariablesService extends AsyncServiceBase {
return evaluator.fullExpression.replace(/`/gi, "");
}

// Do not evaluate if the appropriate context is not available
Copy link
Collaborator Author

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 before itemContext 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 the itemContext, @item._index1 was evaluated with the value of @item_index replaced. For the item at index 0, this actually meant the expression evaluated as 01, 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...

Copy link
Member

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

if (type === "item" && !context.itemContext) {
return evaluator.fullExpression;
}

// process the main lookup, e.g. @local.some_val, @campaign.some_val
// NOTE - if parse fail an empty string will be returned
let { parsedValue, parseSuccess } = await this.processDynamicEvaluator(evaluator, context);
Expand Down
Loading
Loading