Skip to content

Remove the tree viewset#2221

Merged
nucleogenesis merged 28 commits into
learningequality:developfrom
rtibbles:imalumberjackandimokay
Sep 9, 2020
Merged

Remove the tree viewset#2221
nucleogenesis merged 28 commits into
learningequality:developfrom
rtibbles:imalumberjackandimokay

Conversation

@rtibbles

@rtibbles rtibbles commented Sep 4, 2020

Copy link
Copy Markdown
Member

Description

  • The relatively straight forward proposition of this PR is to remove the tree viewset and just go back to querying contentnodes
  • This is not so straight forward as much of the frontend and backend code is built predicated on this separation
  • Refactor all frontend ContentNode vuex state to just work directly with ContentNodes and not separate tree node representations
  • Update all copy operations for ContentNodes in the tree to work on just a ContentNode
  • Update resource loading on the channel edit page
  • Switch caching strategy from matching indexedDB queries to parameter specific in order to prevent synchronous requests from hitting the backend twice.
  • Update the clipboard to no longer rely on the Tree
  • Create a separate Clipboard viewset to handle incremental loading of the clipboard and node creation within the clipboard
  • Update the clipboard copy strategy to only copy children when they differ from the original node children
  • Dynamically load children in the clipboard depending on whether it is a deep representation
  • Add logging messages in the resource layer to allow for easier stack traces to understand where async calls originate
  • Update the sync code to sync more frequently

Issue Addressed (if applicable)

Fixes #2105
Fixes #1941
Fixes #1882

Steps to Test

  • Check that channels with large trees load channel edit
  • Check that you can copy content nodes (caveat - assessmentitem file copying is currently broken from a pre-existing issue)
  • Check that you can copy content nodes to and from the clipboard

@rtibbles rtibbles added this to the Vue Refactor milestone Sep 4, 2020
@nucleogenesis

Copy link
Copy Markdown
Contributor

Possibly unrelated: Removing a Topic (only topics as far as I can see) from a channel looks like it works at first (snackbar is good, node removed from node panel) but on page reload after a few seconds, it reappears. Content items are moved to trash as expected.

@nucleogenesis

Copy link
Copy Markdown
Contributor

Great work @rtibbles this is a lot. The JS/Vue looked good to me. I noted a couple issues above and will be happy to test again whenever you'd like another pass.

@rtibbles
rtibbles force-pushed the imalumberjackandimokay branch from 529e5aa to b23fd7a Compare September 4, 2020 18:17
@rtibbles
rtibbles marked this pull request as ready for review September 4, 2020 18:18
@rtibbles

rtibbles commented Sep 4, 2020

Copy link
Copy Markdown
Member Author

Have resolved test failures and issues observed from manual testing, think this is ready for broader review now.

@rtibbles

rtibbles commented Sep 4, 2020

Copy link
Copy Markdown
Member Author

One last linting fix.

@codecov

codecov Bot commented Sep 4, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2221 into develop will increase coverage by 0.28%.
The diff coverage is 62.29%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2221      +/-   ##
===========================================
+ Coverage    80.77%   81.05%   +0.28%     
===========================================
  Files          289      289              
  Lines        14041    14000      -41     
===========================================
+ Hits         11341    11348       +7     
+ Misses        2700     2652      -48     
Impacted Files Coverage Δ
contentcuration/contentcuration/views/base.py 55.89% <ø> (+0.38%) ⬆️
contentcuration/contentcuration/viewsets/base.py 73.28% <33.33%> (-2.98%) ⬇️
contentcuration/search/viewsets/contentnode.py 59.13% <50.00%> (+0.62%) ⬆️
...ntcuration/contentcuration/viewsets/contentnode.py 72.60% <55.55%> (-9.61%) ⬇️
...tentcuration/contentcuration/viewsets/clipboard.py 70.37% <70.37%> (ø)
...contentcuration/tests/viewsets/test_contentnode.py 86.55% <91.30%> (+0.19%) ⬆️
contentcuration/contentcuration/urls.py 93.12% <100.00%> (ø)
...uration/contentcuration/viewsets/sync/constants.py 100.00% <100.00%> (ø)
...curation/contentcuration/viewsets/sync/endpoint.py 83.47% <100.00%> (ø)
... and 1 more

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 c1d9353...9fbe447. Read the comment docs.

@rtibbles

rtibbles commented Sep 4, 2020

Copy link
Copy Markdown
Member Author

OK, for real, that's the last linting fix...

@nucleogenesis nucleogenesis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rtibbles Fixes work for me and lent to me finding some new fun bugs in the clipboard (not related to this PR)!

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

Aww I kind of miss the tree data structure... Oh well. Looks good to me.

Comment thread contentcuration/contentcuration/frontend/shared/data/changes.js
Comment thread contentcuration/contentcuration/frontend/shared/data/resources.js Outdated

@nucleogenesis nucleogenesis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've found some more issues that may be related to the PR.

In the following (some may not be related to PR):

  • Topic says 6 items, opens to show none.
  • Content items are indented related to topics - looking like children of a node they are not children of.
  • The only time the context menu options appear at the top of the clipboard drawer is when a whole channel's clipboard node is selected.
  • In NodePanel, selecting more than one item and copying it doesn't copy everything.

weird-clipboard

This happened once:

  • Have nodes copied to clipboard.
  • Delete those nodes in the clipboard panel
  • See the nodes in clipboard panel and node panel get deleted... :(

I did this once and the nodes were deleted in NodePanel and Clipboard but when I refreshed they came back. So I decided to record doing it again for this and when the nodes got deleted in both places it was permanent.

toomanydeletions

@bjester

bjester commented Sep 4, 2020

Copy link
Copy Markdown
Member

@nucleogenesis Ah yeah, doesn't look like the clipboard selection handling is working correctly, at the least. It should look like this when a deep node is selected. Admittedly, I know I left 1 or 2 bugs with the selection logic, but they're not obvious.

Screenshot from 2020-09-04 16-39-32

@rtibbles Let me know if you want any eyes on the selection logic.

@MisRob
MisRob removed their request for review September 8, 2020 12:30
@rtibbles

rtibbles commented Sep 8, 2020

Copy link
Copy Markdown
Member Author

I have fixed the deletion issues - there is one extant thing that I am aware of to do with descendant counts in the clipboard.

Also, I noticed that toggling a topic's selection toggles through:

  • Topic selected and all descendants selected
  • Topic indeterminate and all descendants selected
  • Topic selected and all descendants unselected
  • Topic unselected and all descendants unselected

I tried to fix this, but the bit maps made my brain hurt. As it is pre-existing, I'd rather leave it.

@rtibbles

rtibbles commented Sep 9, 2020

Copy link
Copy Markdown
Member Author

Going to rebase to address merge conflicts.

@rtibbles
rtibbles force-pushed the imalumberjackandimokay branch from a14704c to 70c7657 Compare September 9, 2020 00:38
@rtibbles

rtibbles commented Sep 9, 2020

Copy link
Copy Markdown
Member Author

Rebased.

@rtibbles

rtibbles commented Sep 9, 2020

Copy link
Copy Markdown
Member Author

@nucleogenesis I think I have addressed most of your issues, but

Topic says 6 items, opens to show none.

May still be extant in places, depending on the exact sequence of actions you take. Would like to come back to that later as part of some other work.

@nucleogenesis

nucleogenesis commented Sep 9, 2020

Copy link
Copy Markdown
Contributor
  1. GET http://localhost:8080/api/contentnode?parent=53ffb3c1131c47dbaa11ddafcf01710a -> 412 Precondition Failed -> {"detail":"No valid filter parameters supplied"}

First, I got this when I 'duplicated / made a copy of' a topic folder that had some contents in it. When I navigated to that copy, I get the above error. Note that this is unrelated to the clipboard itself, but perhaps shares some internals? I cannot replicate using this method, but can with the following:

Replicate by copying a topic into another topic - then "go to location" from the snackbar message to where you moved the topic. Similarly to item 5 below, the "Move to where..." modal reflects what my moving behavior suggests it ought to. The changes are just not being reflected in NodePanel when navigating and I get the above error when I try to. It does however show in the TreeView sidebar.


  1. One of my pre-existing clipboard nodes no longer appears to exist. I cannot select it in the Clipboard, which prevents my selecting the entire channel that the bad node lives in.

Fixed when item 3 is fixed below


  1. Whenever I try to open the clipboard after adding nodes, I get this error. It seems that I've now gotten my clipboard into an unrecoverable broken state by having added some nodes to it.
TypeError: Cannot read property 'excluded_descendants' of null
    at getters.js:77
    at Array.filter (<anonymous>)
    at Object.<anonymous> (getters.js:77)
    at Object.<anonymous> (getters.js:303)
    at getters.js:354
    at Array.map (<anonymous>)
    at Object.<anonymous> (getters.js:354)
    at VueComponent.<anonymous> (getters.js:312)
    at VueComponent.selectionState (mixins.js:33)
    at Watcher.get (vue.runtime.esm.js:4479)

Unless there is an underlying issue with that property not being set in the first place, we just need to handle the case where the excluded_descendants isn't yet set. I fixed it locally using lodash/get and appears to work now. Nodes that I tried to copy before it was "broken" that never showed are now showing. This also fixes #2 above as I was able to select that node with this in place:

import get from 'lodash/get';

...

if(ancestor) {
    return children.filter(c => get(ancestor, 'extra_fields.excluded_descendants[c.id]')); 
}

  1. I moved a video from root tree node to a newly created subtopic, but the move is not reflected in either place. The video was a recently duplicated content node. I also got a 400 on the sync endpoint relatively close to trying this and prettified the payload and response here: https://gist.github.com/nucleogenesis/21769a23a812ce5c8ff17b9d9e85d18e

  1. I copied from Channel A to Clipboard and when I went to insert into Channel B (an entirely empty channel) it did not show in the NodePanel. However, when I go to move another node from Clipboard -> Channel B again, the first file I moved there is showing in Channel B's root on the "Select where you want to move this clipboard node" page. On the Channel List page, I see the proper resource count (I copied two files there) but they are not showing in the NodePanel after several refreshes and navigating away and back.

Seems only to be an issue of putting nodes in the channel root when it is empty. Otherwise, it appears to work as expected.


Also note that the last 2 items were tested with my fix for the excluded_descendants in place - so if your solution differs fundamentally, then I'll revisit these once fixed.

@nucleogenesis nucleogenesis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Follow up issues for those not addressed in my last comment:

  • #2242 (412 error & nodepanel not reflecting changes properly)

@nucleogenesis
nucleogenesis merged commit 22f5004 into learningequality:develop Sep 9, 2020
@rtibbles
rtibbles deleted the imalumberjackandimokay branch September 9, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants