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

refactor: store partial updates in dynamic data cache #2815

Merged
merged 6 commits into from
Feb 28, 2025

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Feb 24, 2025

PR Checklist

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

Description

Following issue mentioned in #2812 and mattermost thread, update dynamic data to only store partial updates in cache. This will in-turn reduce the amount of data that is synced to the server

Author Notes

This should mean if inspecting indexeddb locally that the data is slightly more easy to interpret (see screenshots below). It may be a bit confusing in cases where the initial list has data that varies, e.g. in the example feat_data_actions example row 1 is saved as complete: false, whist rows 2 and 3 are complete: true - so marking all as complete only stores data for row 1

Review Notes

Testing with feat_data_actions should show that data is stored in a minimal state, but still displays as expected after app reload.

I haven't tested restoring user profile from partial data, although I would expect it to behave in the same way as before as it calls the same data update methods from the dynamic-data service (would still be good to double-check).

Likewise I haven't tested examples where new data rows have been added via add_data action, as I couldn't figure out which example/feat/debug sheet had examples to work with

Dev Notes

I haven't attempt to migrate existing data to the partial format as I feel this could get a bit messy (would need to iterate over all existing indexeddb rows, diff against original sheet and re-save). Any existing data that already contains full row information will remain that way unless the same row is updated, however having both forms won't be incompatible as one is a subset of the other, and both would be merged with the full row when displayed in the app.

For mixed data on the server, it would be possible to perform a one-time cleaning if useful, although I expect having excessive data shouldn't really cause much issue.

Git Issues

Addresses (1) in #2812 - issue should be renamed/edited post merge

Closes #

Screenshots/Videos

After - Only partial row changes stored in value data

NOTE - when data is synced with the server it is the inner data that is synced, so this should now be much more minimal

image

@github-actions github-actions bot added the maintenance Core updates, refactoring and code quality improvements label Feb 24, 2025
@chrismclarke chrismclarke marked this pull request as ready for review February 24, 2025 20:59
@github-actions github-actions bot added maintenance Core updates, refactoring and code quality improvements and removed maintenance Core updates, refactoring and code quality improvements labels Feb 24, 2025
Copy link
Collaborator

@jfmcquade jfmcquade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chrismclarke, nice that the code changes are minimal.

I've tested making dynamic data changes via the feat_data_actions template, then restoring profile data via the user: import action, and can confirm that the partial data appears to be merged correctly.

Copy link
Collaborator

@esmeetewinkel esmeetewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id column in the view on the dynamic data within the user page seems to have disappeared at least in some cases...

To reproduce: e.g.

  • go to the template feat_data_actions
  • click "mark all complete" (which should set one of the rows as complete, since the others are complete by default)
  • go to \user
  • observe that the table for feat_data_actions_list only a completed column, no id column

image

@esmeetewinkel
Copy link
Collaborator

esmeetewinkel commented Feb 27, 2025

[UPDATE] I believe this is addressed by PR #2824 and was logged as issue #2810

I found the following behaviour, not sure if it's a bug or a feature? I'm a bit scared this might cause undesirable behaviours because this is effectively some sort of data merge (rather than just the imported users data overriding all the previous):

When I import a user whose data in a particular table is

completed
true

to a user who has the same table stored as

completed
false
false
false

then the resulting table is

completed
true
false
false
Screenity.video.-.Feb.27.2025.mp4

@chrismclarke
Copy link
Member Author

chrismclarke commented Feb 27, 2025

The id column in the view on the dynamic data within the user page seems to have disappeared at least in some cases...

To reproduce: e.g.

  • go to the template feat_data_actions
  • click "mark all complete" (which should set one of the rows as complete, since the others are complete by default)
  • go to \user
  • observe that the table for feat_data_actions_list only a completed column, no id column

Ah yes, that makes sense as the table only includes changes and the id is never changed. This only effects the UI of the user debug page (the id is still retained at a higher level), so I've gone ahead an pushed b8713eb to always prepend an id column at the start of the table

There was also an issue due to the way table column heading were extracted by looking at the first item of data only, that for partial data anything in later rows were excluded if not included the first row. I've fixed that with 8217222

So right now the table should at least be a little bit more informative in the way it displays the current list of changed values

image

Although I'm wondering whether it might still be preferable to show the full list of values (default + changed)? I'm guessing the most useful would probably be to only include columns that have changed values (to not show the entire data list), but then use some sort of highlighting/colour to show both the default value where unchanged. Maybe something like:

image

In either case I don't think would need to be a blocker on this PR, could create follow-up issue. Similarly the minor code updates I just pushed shouldn't need technical review.

@chrismclarke
Copy link
Member Author

chrismclarke commented Feb 27, 2025

@esmeetewinkel @jfmcquade
Oh, and also to quickly mention I additionally tested setting data on the main branch, moving over to this PR branch and continuing to update. Everything still displayed as expected (no changes lost), although just to note that legacy data does in fact keep the full row entry even after partial changes.

So if we want to fully clean-up legacy data in the future we would need a database migration to do so. For now it means some users will still continue to upload larger amounts of data where full data_list rows have been stored in the dynamic data system. This would involve looking at all data that has been saved, cross-checking against original sheet data and removing entries where the data should be unchanged. Again this should likely be a follow-up issue, as what is in this PR wouldn't change how that is done in the future

@esmeetewinkel
Copy link
Collaborator

Follow up issue: #2825

@esmeetewinkel esmeetewinkel self-requested a review February 28, 2025 08:56
Copy link
Collaborator

@esmeetewinkel esmeetewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional test passed

image

@esmeetewinkel esmeetewinkel merged commit 8221604 into master Feb 28, 2025
6 checks passed
@esmeetewinkel esmeetewinkel deleted the feat/dynamic-data-partial-cache branch February 28, 2025 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Core updates, refactoring and code quality improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants