IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

fix: data-items underscore properties #2323

Closed chrismclarke closed 1 month ago

chrismclarke commented 1 month ago

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

PR Checklist

Description

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 image

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

jfmcquade commented 1 month ago

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 commented 4 weeks ago

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