') + ')', '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); } })(); })(); `complete` validation improvements by MisRob · Pull Request #2606 · learningequality/studio · GitHub
Skip to content

complete validation improvements - #2606

Merged
rtibbles merged 5 commits into
learningequality:developfrom
MisRob:improve-node-complete
Dec 9, 2020
Merged

complete validation improvements#2606
rtibbles merged 5 commits into
learningequality:developfrom
MisRob:improve-node-complete

Conversation

@MisRob

@MisRobMisRob commented Dec 1, 2020

Copy link
Copy Markdown
Member

Description

Make complete validation more robust:

  • require assessment items or files to be defined for certain kinds of nodes before running evaluation function
  • set complete in corresponding actions in one transaction together with the related node updates
  • improve validation functions naming to avoid confusion about return values that we experienced during our previous work

Issue Addressed

Closes#2482

Steps to Test

  • Navigate to edit modal
  • Edit a node and check that a correct complete is sent in /api/sync request whenever its value changes. Validation rules are listed here.

Implementation Notes

After playing with several approaches, including forcing complete to be present on each related Resource's updates, I ended up using what @rtibbles initially suggested, and that is to rather evaluate and set complete in corresponding actions. This solution has shown to be more flexible than other approaches that usually ended up with lots of unnecessary code in our views for operations that do not even affect the completeness of a node. As long as we run this evaluation on clients only, we'll need to be careful about not forgetting to have all data available, whatever strategy used.

Checklist

  • Is the code clean and well-commented?
  • Has the docs label been added if this introduces a change that needs to be updated in the user docs?
  • Has the CHANGELOG label been added to this pull request? Items with this label will be added to the CHANGELOG at a later time
  • Are there tests for this change?
  • Are all user-facing strings translated properly (if applicable)?
  • Has 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)?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Are views organized into pages, components, and layouts directories as described in the docs?
  • Are there any new ways this uses user data that needs to be factored into our Privacy Policy?
  • Are there any new interactions that need to be added to the QA Sheet?
  • Are there opportunities for using Google Analytics here (if applicable)?
  • If the Pipfile has been changed, is the updated Pipfile.lock file also included in this PR?
  • Are the migrations safe for a large db (if applicable)?

@MisRob
MisRob requested a review from rtibblesDecember 1, 2020 12:05
@codecov

codecovBot commented Dec 1, 2020•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2606 (633911d) into develop (fc35107) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## develop #2606 +/- ##
========================================
Coverage 84.56% 84.56% ========================================
Files 296 296 Lines 15093 15093 ========================================
Hits 12764 12764 Misses 2329 2329
Impacted FilesCoverage Δ
contentcuration/contentcuration/utils/db_tools.py69.75% <100.00%> (ø)

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 533b62b...633911d. Read the comment docs.

MisRob
MisRob commented Dec 1, 2020
View reviewed changes
Comment threadcontentcuration/contentcuration/frontend/channelEdit/components/AssessmentTab/AssessmentTab.vue
},
},
watch: {
assessmentItems(newAssessmentItems) {

@MisRobMisRobDec 1, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Although I am not 100% sure, I assume that this watcher might be the cause of some inconsistencies we've been experiencing lately because (1) it updates a content node in another tick, and (2) the update is run in another transaction than updates that triggered complete re-evaluation.

I haven't closed any related issues I am assigned to though. I'd like to test everything at once after @micahscopes is done with his updates addressing some other, but possibly related, problems.

@MisRobMisRob mentioned this pull request Dec 2, 2020
Closed
@MisRob

MisRob commented Dec 3, 2020

Copy link
Copy Markdown
MemberAuthor

@micahscopes@rtibbles Let's wait for #2498 to be merged before reviewing this one. I'd like to rebase on top of Micah's work.

@MisRob
MisRob marked this pull request as draft December 3, 2020 16:29
@MisRobMisRob added the WIP label Dec 3, 2020
@MisRob
MisRobforce-pushed the improve-node-complete branch from 0bab8be to 633911dCompareDecember 7, 2020 06:47
@MisRobMisRob added needs review and removed WIP labels Dec 7, 2020
@MisRob
MisRob marked this pull request as ready for review December 7, 2020 07:10
@MisRob

MisRob commented Dec 7, 2020

Copy link
Copy Markdown
MemberAuthor

@micahscopes I've rebased and resolved some smaller conflicts with #2498. Everything is ready for review.

rtibbles
rtibbles reviewed Dec 9, 2020
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.

Read through of the code changes makes sense to me, and definitely seems like a good change to consolidate this outside of the component code and into the actions. (I just reread your PR comment, and had forgotten that this was my initial suggestion, so I am glad I am consistent at least).

Will do some manual testing before merge.

@marcellamaki

marcellamaki commented Dec 9, 2020

Copy link
Copy Markdown
Member

I've done some manual testing and I think everything is working (at least in my understanding of using Studio so far 😄) - both from the UX point of view and with the true/false values for complete returning as expected.

rtibbles
rtibbles approved these changes Dec 9, 2020
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.

Two half thumbs up make a whole thumbs up!

@rtibbles
rtibbles merged commit 1ad9b8e into learningequality:developDec 9, 2020
@MisRobMisRob mentioned this pull request Dec 10, 2020
Closed
@MisRob
MisRob deleted the improve-node-complete branch February 3, 2021 07:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rtibblesrtibblesrtibbles approved these changes
@micahscopesmicahscopesAwaiting requested review from micahscopes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Complete field follow-ups

3 participants

@MisRob@marcellamaki@rtibbles