Skip to content

Change event log#3134

Merged
bjester merged 58 commits intolearningequality:unstablefrom
rtibbles:async_sync
Jun 27, 2022
Merged

Change event log#3134
bjester merged 58 commits intolearningequality:unstablefrom
rtibbles:async_sync

Conversation

@rtibbles
Copy link
Copy Markdown
Member

@rtibbles rtibbles commented May 3, 2021

Summary

Description of the change(s) you made

  • Implements a change event log on the server side
  • Makes all change application run inside an async task on the server side
  • Separates out user specific and channel specific changes
  • Allows syncing back of changes relevant to channels and users
  • Removes Task API and sends task information back via the sync endpoint
  • Separates out Channel bookmarks from the main channel data as changes to it are user specific, whereas changes to channels are channel specific
  • Creates dummy Task objects for Publish, Sync, and Copy changes in order to continue to allow the frontend to track progress of application of these changes
  • Moves publishing and syncing display from a modal into the top toolbar as it is no longer necessary to block user interaction.

Manual verification steps performed

  • Do edits
  • Save edits
  • Confirm they get synced
  • Confirm they get applied
  • Confirm they get removed from the frontend

Reviewer guidance

How can a reviewer test these changes?

Please test the basic flow for now.

Are there any risky areas that deserve extra testing?

EVERYTHING!

References

Fixes #3034
Fixes #2828
Fixes #3101

Comments


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 bjester added this to the 2022Q1 Phase 1: tasks milestone Feb 15, 2022
@rtibbles rtibbles force-pushed the async_sync branch 5 times, most recently from 3faa634 to 0582b91 Compare March 2, 2022 00:07
Copy link
Copy Markdown
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Code review looks good-- left some comments. Let me know if it's ready for testing

@rtibbles rtibbles force-pushed the async_sync branch 2 times, most recently from 2508621 to 0517ace Compare March 22, 2022 16:54
@rtibbles rtibbles marked this pull request as ready for review April 12, 2022 17:44
Copy link
Copy Markdown
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Left more code comments. I'll do some manual QA now

@bjester
Copy link
Copy Markdown
Member

bjester commented Apr 26, 2022

I saw this error while creating a new channel

django.core.exceptions.FieldError: Cannot resolve keyword 'can_edit' into field. Choices are: auth_token, bookmarked_channels, changes_about_user, changes_by_user, channel_sets, clipboard_tree, clipboard_tree_id, content_defaults, date_joined, disk_space, disk_space_used, editable_channels, email, feature_flags, files, first_name, groups, id, information, is_active, is_admin, is_staff, is_superuser, last_login, last_name, logentry, password, policies, preferences, searches, sent_by, sent_to, staged_files, task, user_permissions, view_only_channels

@bjester
Copy link
Copy Markdown
Member

bjester commented Apr 26, 2022

Seems the edit modal Saving... indicator is always visible now, even after the changes have synced.

@rtibbles
Copy link
Copy Markdown
Member Author

Seems the edit modal Saving... indicator is always visible now, even after the changes have synced.

Ah yeah, we'll need to update that check to filter out 'applied' changes.

@rtibbles
Copy link
Copy Markdown
Member Author

I saw this error while creating a new channel

Thanks, I'll track this down.

@rtibbles
Copy link
Copy Markdown
Member Author

Hrm, where were you creating a channel from? I just tried it myself, and didn't replicate this error.

@bjester
Copy link
Copy Markdown
Member

bjester commented Apr 28, 2022

Hrm, where were you creating a channel from? I just tried it myself, and didn't replicate this error.

I created it from the channels list page. Perhaps that error occurred prior to actually creating it. I can try again later

@rtibbles
Copy link
Copy Markdown
Member Author

After publishing, the publish button became enabled again, even though no changes were made. New nodes that were unpublished are still marked as unpublished after publishing.

The publish button should be properly toggling now, and all content nodes in the frontend are properly flagged as published and unchanged.

I did this by switching to tracking for edits since the last publish (except in the case of ricecooker channels, where I fall back to the changed attribute).

the new metadata fields, both on folders and on resource editing

This was very reproducible and was a result of my poor decision to use dot paths in the materialized paths of our nested metadata labels. This confuses Dexie which also uses dot paths, but for describing nested keyPaths for changes. I have monkey patched Dexie to add specific excludes for our metadata labels to prevent this from causing an issue.

I can't fully tell if it's related to these changes, is the content library is not actually displaying the resources, just the number of results

This was a bug I introduced while refactoring the bookmark code - there was an error in the getChannels getter function - which I have now corrected.

In addition, I have done some testing of this across two browsers, and it seems like the change application logic across browsers is working as intended - I did this after the issues that @marcellamaki had reported, to ensure that the fix I applied to Dexie was universal. I retested this after the upgrade to Dexie 3.2 and the fix persisted.

Copy link
Copy Markdown
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Calling it good!

@bjester bjester requested review from marcellamaki and removed request for micahscopes June 27, 2022 16:07
@bjester
Copy link
Copy Markdown
Member

bjester commented Jun 27, 2022

Ran the python tests manually.

@bjester bjester merged commit 42081ba into learningequality:unstable Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants