') + ')', '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); } })(); })(); Filter channelsetlist by user by marcellamaki · Pull Request #2613 · learningequality/studio · GitHub
Skip to content

Filter channelsetlist by user - #2613

Merged
rtibbles merged 4 commits into
learningequality:developfrom
marcellamaki:filter-channelsetlist-by-user
Dec 4, 2020
Merged

Filter channelsetlist by user#2613
rtibbles merged 4 commits into
learningequality:developfrom
marcellamaki:filter-channelsetlist-by-user

Conversation

@marcellamaki

Copy link
Copy Markdown
Member

What does this PR do? Briefly describe in 1-2 sentences

This PR filters the ChannelSetList (aka "Collections") so that only the current user's Collections are displayed.

Issue Addressed (if applicable)

Addresses #2542 - Admins can see all Channelsets/Collections, even if they do not have edit access to them

Steps to Test

  • Sign in as Admin and ensure you have at least one collection
  • Create a second test account that is also an Admin and sign in. You should not see the other Admin collections.
  • Create a collection. You should see it displayed
  • Sign in as the original Admin. You should only see your collections still

At a high level, how did you implement this?

I created a FilterSet on the backend for ChannelSetList and passed a parameter in the front end .where() query and updated tests for this change.

@codecov

codecovBot commented Dec 2, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2613 (3fe77cd) into develop (31eed50) will decrease coverage by 0.04%.
The diff coverage is 95.83%.

Impacted file tree graph

@@ Coverage Diff @@## develop #2613 +/- ##
===========================================
- Coverage 84.61% 84.56% -0.05% 
===========================================
Files 295 296 +1 Lines 15018 15093 +75 ===========================================
+ Hits 12707 12764 +57 - Misses 2311 2329 +18 
Impacted FilesCoverage Δ
...entcuration/contentcuration/viewsets/channelset.py96.00% <95.23%> (-0.37%)⬇️
contentcuration/contentcuration/models.py85.64% <100.00%> (+0.02%)⬆️
contentcuration/contentcuration/viewsets/base.py88.63% <0.00%> (-2.50%)⬇️
...ntcuration/contentcuration/viewsets/contentnode.py83.43% <0.00%> (-0.42%)⬇️
...uration/contentcuration/tests/test_contentnodes.py94.79% <0.00%> (-0.39%)⬇️
...curation/contentcuration/viewsets/sync/endpoint.py90.24% <0.00%> (-0.31%)⬇️
...tentcuration/migrations/0124_auto_20201204_1645.py100.00% <0.00%> (ø)
...contentcuration/tests/viewsets/test_contentnode.py83.58% <0.00%> (+0.20%)⬆️
...entcuration/contentcuration/viewsets/sync/utils.py93.18% <0.00%> (+1.51%)⬆️

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 b9bcc9f...3fe77cd. Read the comment docs.

@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.

Suggestion for a slight change of approach.

filter_class = ChannelSetFilter
permission_classes = [IsAuthenticated]
values = ("id", "name", "description", "channels", "secret_token__token")
values = ("id", "name", "description", "channels", "secret_token__token", "editors")

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.

There is an interesting gotcha here, where adding editors to the values call, because editors is a many to many field, if there is more than one editor, then this will cause the same channelset to be returned multiple times in the resultant query (each with a different editors value corresponding to each editor).

Although currently we have no way in the interface of adding additional editors to a collection, I imagine we will do so at some point in the future.

I would instead do this consistently with how we filter Channels, and instead annotate an edit flag on the viewset, which is then different on a per user basis.

This is annotated onto the Channels queryset here: https://github.com/learningequality/studio/blob/develop/contentcuration/contentcuration/models.py#L780 (this method is called for getting the queryset inside these viewsets, so doing something parallel in the method on ChannelSet would mean this annotation can be used in the FilterSet and returned as a value).

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.

Thanks for the feedback - I've made some updates based on these notes and our conversation yesterday, and I think it's now using the approach you suggest. Let me know if you have any other comments!

@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.

One last change.

queryset = super(ChannelSetViewSet, self).get_queryset()
user_id = not self.request.user.is_anonymous() and self.request.user.id
edit = Exists(User.channel_sets.through.objects.filter(user_id=user_id, channelset_id=OuterRef("id")))
return queryset.annotate(edit=edit).filter(edit=True)

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.

Should remove this: .filter(edit=True) otherwise we are building in the filter you've added here into the base queryset.

Beyond that, this is good to go!

@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.

Manually tested, filtering is working as intended.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@marcellamaki@rtibbles