') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Ensures nodes are loaded prior to copy, a use case for importing from search by bjester · Pull Request #2858 · learningequality/studio · GitHub
Skip to content

Ensures nodes are loaded prior to copy, a use case for importing from search - #2858

Merged
bjester merged 9 commits into
learningequality:hotfixesfrom
bjester:source-node-import-channel
Jan 28, 2021
Merged

Ensures nodes are loaded prior to copy, a use case for importing from search#2858
bjester merged 9 commits into
learningequality:hotfixesfrom
bjester:source-node-import-channel

Conversation

@bjester

@bjesterbjester commented Jan 22, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Description

The content node copy operation was assuming the relevant nodes were already loaded, which in the case of importing from search, wasn't true.

This PR unwraps code out of a transaction such that we can use our resource methods that will attempt to retrieve the object from our API if it's not present locally or is stale. This required unwrapping it because the API response must be inserted into the IndexedDB. To prevent against edge cases that could cause inconsistencies in rapid calls to ContentNode.copy or .move, I added a small in-memory locking memory to ensure that if it does retrieve objects from the API, the operations should still happen in order.

Issue Addressed (if applicable)

Resolves: #2820

Steps to Test

Preconditions: A channel created.

  • Login to Studio
  • Click a channel to load the channel editor page
  • Click the Add button
  • Click Import from other channels
  • Do a search in the search field ("Science" in this specific case)
  • Checkbox-select some topics/resources
  • Click the Review button
  • Click the Import button

Checklist

  • Is the code clean and well-commented?
  • Are there tests for this change?

rtibbles
rtibbles reviewed Jan 22, 2021
View reviewed changes

@rtibblesrtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code changes look good to me. Have not manually tested.

bjester
bjester commented Jan 22, 2021
View reviewed changes
Comment threadcontentcuration/contentcuration/frontend/shared/data/resources.js Outdated
@bjester
bjester requested a review from jayoshihJanuary 22, 2021 19:20
@rtibbles

rtibbles commented Jan 22, 2021

Copy link
Copy Markdown
Member

Thanks for adding the tests!

rtibbles
rtibbles reviewed Jan 22, 2021
View reviewed changes

@rtibblesrtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

New code changes make sense, one comment.

Comment threadcontentcuration/contentcuration/frontend/shared/data/__tests__/ContentNodeResource.spec.js Outdated
rtibbles
rtibbles approved these changes Jan 22, 2021
View reviewed changes

@rtibblesrtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Manual test checks out. This is good to merge.

rtibbles
rtibbles requested changes Jan 23, 2021
View reviewed changes

@rtibblesrtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oof, we've noticed a Dexie transaction thing going on. So we need to hold off merging for now!

@MisRob

MisRob commented Jan 25, 2021

Copy link
Copy Markdown
Member

I'm not sure what's the problem with the Dexie transaction, but at least had a look at code and changes make sense to me.

@rtibbles

rtibbles commented Jan 25, 2021

Copy link
Copy Markdown
Member

We see this error when the frontend has to fetch the required data from the backend (but not when the data is already in the IndexedDB table):

DexieError "Transaction has already completed or failed"

@bjester

bjester commented Jan 25, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks @rtibbles and @MisRob , I made a mistake in my test that didn't capture the fact that fetching a model would then put it into IndexedDB, which requires a transaction to set the source as fetch. I'll work on refactoring to get around this, and add more tests!

@bjesterbjester changed the title Ensures nodes are loaded prior to copy, a use case for importing from searchWIP: Ensures nodes are loaded prior to copy, a use case for importing from searchJan 26, 2021
@codecov

codecovBot commented Jan 27, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2858 (1cf913d) into hotfixes (5fd66f8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## hotfixes #2858 +/- ##
=========================================
Coverage 84.97% 84.97% =========================================
Files 295 295 Lines 15558 15559 +1 =========================================
+ Hits 13220 13221 +1 
Misses 2338 2338
Impacted FilesCoverage Δ
contentcuration/contentcuration/tasks.py62.80% <100.00%> (ø)
...ntcuration/contentcuration/tests/test_asynctask.py93.52% <100.00%> (+0.04%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fd66f8...c56e002. Read the comment docs.

@bjesterbjester changed the title WIP: Ensures nodes are loaded prior to copy, a use case for importing from searchEnsures nodes are loaded prior to copy, a use case for importing from searchJan 27, 2021
@bjesterbjester removed the WIP label Jan 27, 2021
@bjester
bjester requested a review from rtibblesJanuary 27, 2021 19:59
@marcellamaki

marcellamaki commented Jan 27, 2021

Copy link
Copy Markdown
Member

Manual testing looks good - I'm not getting any Dexie errors!

rtibbles
rtibbles approved these changes Jan 27, 2021
View reviewed changes

@rtibblesrtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Couple of things that I noticed, but none of them are blockers.

Comment threadcontentcuration/contentcuration/frontend/shared/data/resources.js
/**
* Tree resources mixin
*/
class TreeResource extends Resource {

@rtibblesrtibblesJan 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could avoid the subclassing here and just pass treeLock method as an option to the Resource constructor for ContentNode - and add in an existence check for this._locks - but class extension works too!

Comment threadcontentcuration/contentcuration/frontend/shared/data/resources.js Outdated
Comment threadcontentcuration/contentcuration/frontend/shared/data/resources.js Outdated
@bjester
bjester merged commit c207187 into learningequality:hotfixesJan 28, 2021
@bjester
bjester deleted the source-node-import-channel branch April 27, 2021 20:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rtibblesrtibblesrtibbles approved these changes
@jayoshihjayoshihAwaiting requested review from jayoshih
@MisRobMisRobAwaiting requested review from MisRob

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Post Release Stabilization

Development

Successfully merging this pull request may close these issues.

Import from other channels - Import from Search results is not working

4 participants

@bjester@rtibbles@MisRob@marcellamaki