fix: data-items underscore properties

[chrismclarke]

Add support for storing data-items with underscore properties (previously rejected by rxdb regex)

PR Checklist

• PR title descriptive (can be used in release notes)

Description

• Update dynamic-data service to support using data lists that include _ prefixed fields (e.g. _translations)
• Add tests

Dev Notes

I looked at initially adding a new prefix to all _ prefixed fields and then extracting back out but it seemed a little more efficient to just combine in a single top-level property. The only downside to doing things this way (that I'm aware of) is losing the rxdb ability to query on the field, although given these are mostly computed metadata fields I don't think that's overly useful anyways.

I've only added handling for populating the _ fields as part of initial data list read, and not as part of dynamic data write operations. This means that authors won't be able to use _ fields as part of set_item operations. Whilst we could do similar merging to support I'm assuming treating these more as readonly probably makes more sense for now anyways.

The write operation will not show any error, just silently ignore the metadata field as it is not included as part of the rxdb schema. If we wanted to keep the values we would need to extract and include in deep-merge.

I can't remember what was agreed when discussing #2312 - do we also want to implement in a similar way or just leave as-is for now (would possibly involve another migration to revert back to an underscore prefix field)

Author Notes

This shouldn't change much from an authoring perspective as everything is handled internally, just to be aware that there is a new APP_META prefix used to temporarily combine all other _ fields and so should avoid using the same column name in any data lists.

It may be worth keeping an eye out for how the translated fields in data-items behave when swapping between translations (as this is the metadata field causing issues). I haven't included any explicit tests for this right now (more just making sure the data can be stored)

Git Issues

Closes #2310

Screenshots/Videos

Before - newly created test detects error as identified in issue

After - 2 new tests added and passing to support reading data with _ fields and ignoring writes

[esmeetewinkel]

Functional test passed.

Noted about not using APP_META as a column header

[jfmcquade]

I can't remember what was agreed when discussing #2312 - do we also want to implement in a similar way or just leave as-is for now (would possibly involve another migration to revert back to an underscore prefix field)

The combination of these two sets of changes is a little confusing because both deal with "meta" but handle it in a different way. However, they are compatible and couldn't trivially combined: as of #2312, we querying on the row_index field and so, as you note above, this field couldn't be included in the APP_META property (without also sorting on this field post-query, which would presumably impact performance). So I think fine to leave row_index as is for now?

It is also worth mentioning #2215. As of that PR, we use the _ prefix for index and id in the set_item action params to indicate that these are properties to be used to target particular rows for update rather than names of properties to be updated for a given row. Again, this is compatible with the implementation in this PR because we're not trying to allow users to update any properties with an underscore prefix. So I think this should all be good, but something to be aware of if we ever do want authors to be able to update this metadata fields.

Along these lines, whilst we may not want authors to update these fields directly, we may want to update them programatically at some point, e.g. when importing a new "content pack" or similar.

Anyway, these changes look good to me, I've added one question inline.

I also added a button to add a dynamic data write on the data looped over in debug_data_items_translate in order to manually inspect the dynamic data stored in IndexedDB. It appears as expected:

[jfmcquade]

Don't we want to keep [key: string]: any to capture the fact that the doc will have other fields?

[chrismclarke]

I think it can be argued either way - my slight preference for not emphasising the generic fields is loss of type-safety when trying to refer to the doc in code. e.g. if we changed APP_META to be something else like APP_PROTECTED typescript won't catch the error in other components that refer to the code (as the previous APP_META field would still be allowed). So here the typescript is less trying to accurately represent the data, more representing the subset of data that will be accessed directly within code. If we later want to impose more fields for certain types of datalists we can just extend the base class to keep explicit as required.

[jfmcquade]

Ah yeh, that makes sense. I was only seeing the typescript's purpose as a way of accurately representing the data, but I can see how it's more useful in cases like this to have the type-safety that comes with just representing a subset of the data.

[jfmcquade]

I'm going to merge this as there are no blocking issues, but tagging @chrismclarke to be aware of the discussion points above at some point

[chrismclarke]

Thanks for the review both

@jfmcquade - makes sense what you said about the slightly conflicting-yet-compatible updates, agreed for now makes sense to keep all as they address slightly different use cases and I think trying to tidy might just end up introducing yet another layer of confusion