Localize change sync operations by rtibbles · Pull Request #4070 · learningequality/studio

rtibbles · 2023-05-10T23:06:07Z

Summary

Description of the change(s) you made

Attempts to prevent issues with background tabs sleeping timers and saving memory by making each tab responsible for the creation and persistence to the backend of edits made in the tab context
Creates Change classes mapping to each Change type to handle proper instantiation of the change and persistence to the indexedDB changes table
Updates all put operations to be add operations (new in Dexie v3) to make explicit create vs update changes
Adds change creation logic into add, update, and delete methods
Updates existing move, copy, publish, sync, and deploy changes to use the new Change classes
Removes unused methods on the resource class that modify tables, to prevent future developers from using them without the appropriate change updates
Updates the server sync logic to run in every open tab (but only to poll when the window is visible)
Updates the debounce behaviour to remove need for the syncActive flag, and instead chain from previous invocations
Updates the debounce behaviour to always return a promise that will resolve with the next sync
Updates the forceServerSync function to try to persist all currently unsynced changes, as a last resort before someone logs out.
Activates the service worker to be active during development (this was not ultimately needed for the implementation, and can be dropped if not wanted)

Manual verification steps performed

Make edits to nodes
Upload new nodes
Open multiple tabs, make changes in each, ensure that changes from each tab sync from within the tab

Reviewer guidance

Are there any risky areas that deserve extra testing?

I think the main area for concern here is that it feels like there is more of a chance of a change not getting synced, because a page could be refreshed etc. Not sure how to make sure changes do get synced, while also maintaining the separation of tabs syncing their own changes.

References

Hopefully fixes #3992

Contributor's Checklist

PR process:

If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
If this includes an internal dependency change, a link to the diff is provided
The docs label has been added if this introduces a change that needs to be updated in the user docs?
If any Python requirements have changed, the updated requirements.txt files also included in this PR
Opportunities for using Google Analytics here are noted
Migrations are safe for a large db

Studio-specifc:

All user-facing strings are translated properly
The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
All UI components are LTR and RTL compliant
Views are organized into pages, components, and layouts directories as described in the docs
Users' storage used is recalculated properly on any changes to main tree files
If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

Code is clean and well-commented
Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Any new interactions have been added to the QA Sheet
Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

bjester

Overall, I like this a lot! I left a bunch of comments with my thoughts. My only requested changes though are to fix the service worker issue and to reinstate source tracking, since usages of it are failing on this.

contentcuration/contentcuration/frontend/shared/data/changes.js

bjester · 2023-05-24T20:14:27Z

contentcuration/contentcuration/frontend/shared/data/changes.js

+    this.setAndValidateNotUndefined(target, 'target');
+    this.setAndValidateLookup(position, 'position', RELATIVE_TREE_POSITIONS_LOOKUP);
+    this.setAndValidateNotUndefined(parent, 'parent');
+    this.setChannelAndUserId();


This feels slightly awkward because some indexeddb resources lookup the channel from a passed object, which this supports, but others use getChannelFromChannelScope. Maybe each resource/table should be able to subclass its own change with these specific implementation details. That could make sense as Move and Copy are just applicable to content nodes.

It felt a bit awkward to have TABLE X CHANGE_TYPE changes defined, but I agree that the way this defers to the Resource classes is also awkward.

contentcuration/contentcuration/views/pwa.py

bjester · 2023-05-24T20:33:40Z

contentcuration/contentcuration/frontend/shared/data/changes.js

+    return !isUndefined(this.channel_id) || !isUndefined(this.user_id);
+  }
+
+  setChannelAndUserId(obj) {


No need to change, just conversely to the constructor args, this appears to require the obj parameter when it doesn't always get one! (another comment later on)

bjester · 2023-05-24T20:42:54Z

contentcuration/contentcuration/frontend/shared/data/changes.js

+      throw error;
+    }
+    this[name] = mapper(value);
+  }


I think these setAndValidate* are fine, but another approach could be to set, then validate later, perhaps in saveChange. One benefit would be that the property setting would be more direct, and we could have better IDE visibility in avoiding misspellings in name which must be a string in this approach.

Yeah, I think generally it would be better to separate this - I do like the Vue props convention for specifying types, so it may be just separating out the specification of what value should be what, and setting of it would be good - then just having a validate method that runs through all of them.

bjester · 2023-05-24T20:45:41Z

contentcuration/contentcuration/frontend/shared/data/serverSync.js

-let syncActive = false;
-
 const commonFields = ['type', 'key', 'table', 'rev', 'channel_id', 'user_id'];
-const objectFields = ['objs', 'mods'];


On the topic of misspellings... 💯

contentcuration/contentcuration/frontend/shared/data/serverSync.js

rtibbles · 2023-05-25T23:35:32Z

All blocking comments addressed, and some of the non-blocking ones. I had some issues after rolling back migrations, so have done limited manual testing of the frontend changes, but have added additional automated test coverage.

bjester

Some things have regressed:

Clipboard to topic tree is failing in any manner:

TypeError: Cannot set properties of undefined (setting 'excluded_descendants')
From previous: 
    at Ce.n (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:8:2804215)
    at TreeResource.resolveTreeInsert (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:11:20010)
    at TreeResource.copy (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:11:21939)
    at Store.copyContentNode (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:11:81114)
    at Array.<anonymous> (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:8:2479400)
    at Store.dispatch (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:8:2481965)
    at Store.dispatch (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:8:2475079)
    at local.dispatch (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:8:2477925)

Restoring a node from a trash results in errored change (seems the node vanished from trash too):

{
    "disallowed": [],
    "allowed": [],
    "changes": [],
    "errors": [
        {
            "key": "4109fbff224042dd92352f4b69302531",
            "errors": [
                "[ErrorDetail(string='Specified node does not exist', code='invalid')]"
            ],
            "target": "2c1738cc81004c1785a5a07e24b84733",
            "position": "last-child",
            "server_rev": 250,
            "table": "contentnode",
            "type": 4,
            "channel_id": "02df1a9833f64d279039f71f5045febb",
            "user_id": null,
            "created_by_id": 1
        }
    ],
    "successes": [],
    "tasks": []
}

Using yarn run build and yarn run runserver, after I cleared the site data, this is persisting after refreshing:

Uncaught (in promise) bad-precaching-response: bad-precaching-response :: [{"url":"http://localhost:8080/static/studio/128-128-159012dface3864c747f.js","status":404}]
    at PrecacheStrategy._handleInstall (http://localhost:8080/serviceWorker.js:5:13559)
    at async PrecacheStrategy._handle (http://localhost:8080/serviceWorker.js:5:12588)
    at async PrecacheStrategy._getResponse (http://localhost:8080/serviceWorker.js:5:11149)

Adding a new question to an exercise:

TypeError: (intermediate value).add is not a function
at Resource.add (http://localhost:8080/static/studio/channel_edit-f6c623f34530982ac14f.js:11:27422)

Add more finegrained error catching for applying remote changes.

Always set syncActive to `false` when finished.

…hanges table.

Clean up unused active channel monitoring logic.

…nt is visible.

… object. Ensure we generate changes that require the old object representation before modifying. Ignore null changes.

Update tests with appropriate mocks to handle this.

…mode.

from Dexie Observable.

rtibbles · 2023-05-30T16:45:07Z

I have fixed 1 and 4, but have not been able to replicate 2 or 3.

bjester

I'm still getting (3) but the other things are fixed so I don't know that it's breaking anything! 🎉
Ran through the different edit operations and everything seems good

rtibbles added this to the Kolibri 0.16 Release: Studio Support milestone May 10, 2023

rtibbles requested review from bjester and marcellamaki May 10, 2023 23:06

bjester requested changes May 24, 2023

View reviewed changes

bjester requested changes May 30, 2023

View reviewed changes

rtibbles added 21 commits May 30, 2023 08:32

Add more sentry logging for sync errors.

b53ecc9

Add more finegrained error catching for applying remote changes.

Catch the entire syncChanges function.

d6ab54c

Always set syncActive to `false` when finished.

Update put method to add method to make creates explicit.

abeabb7

Add Change classes to encapsulate and handle setting changes on the c…

c89ba35

…hanges table.

Update syncChanges logic to handle queueing per tab.

7feb5bf

Update forceServerSync to still attempt to sync all unsynced changes.

454bb23

Link up change objects to the resource layer.

fdded78

Clean up unused active channel monitoring logic.

Remove use of leader election. Do background polling only when docume…

3dd6ca3

…nt is visible.

Use serviceWorker during development.

cd49812

Prevent circular dependencies. Use reactive watch to trigger queueing.

ce0bf14

Change updatedChanges to handle an object of changes, not the changed…

6de69b9

… object. Ensure we generate changes that require the old object representation before modifying. Ignore null changes.

Include CHANGES_TABLE in transaction.

e85491a

Properly handle channel_id and user_id setting on changes.

8ec4840

Update tests with appropriate mocks to handle this.

Ensure key is not null.

a73100b

Make the service worker serving handle no webpack devserver in debug …

17ec8ff

…mode.

Make resolve/reject stack more comprehensible.

533b293

Add in saving of the source to the change object.

58c09a6

Update change tracker to only track changes from this client/tab.

f64e6de

Remove use of IGNORED_SOURCE as we are no longer syncing change events

c0e34e4

from Dexie Observable.

Update assessment item handling for change updates.

78e32c3

Fix issues with copying.

05fccf0

rtibbles force-pushed the where_is_my_mind branch from c6385e8 to 05fccf0 Compare May 30, 2023 16:53

bjester approved these changes May 30, 2023

View reviewed changes

bjester merged commit 743364c into learningequality:unstable May 31, 2023

rtibbles deleted the where_is_my_mind branch May 31, 2023 15:13

bjester mentioned this pull request May 31, 2023

Hotfixes release: Kolibri 0.16 support #4002

Merged

bjester mentioned this pull request Jul 5, 2023

Release v2023.07.05 - Kolibri 0.16 support #4187

Merged

Uh oh!

Conversation

rtibbles commented May 10, 2023

Summary

Description of the change(s) you made

Manual verification steps performed

Reviewer guidance

Are there any risky areas that deserve extra testing?

References

Contributor's Checklist

Reviewer's Checklist

This section is for reviewers to fill out.

Uh oh!

bjester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bjester May 24, 2023

Choose a reason for hiding this comment

Uh oh!

rtibbles May 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bjester May 24, 2023

Choose a reason for hiding this comment

Uh oh!

bjester May 24, 2023

Choose a reason for hiding this comment

Uh oh!

rtibbles May 25, 2023

Choose a reason for hiding this comment

Uh oh!

bjester May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rtibbles commented May 25, 2023

Uh oh!

bjester left a comment

Choose a reason for hiding this comment

Uh oh!

rtibbles commented May 30, 2023

Uh oh!

bjester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants