Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1221 - Tapestry 3.0 - Allow standalone nodes #1222

Closed
wants to merge 59 commits into from

Conversation

legendword
Copy link
Contributor

@legendword legendword commented Jul 22, 2022

Changes

  • Make rootId field of the tapestry object optional, so that it is merely used to determine the initial node selected upon viewing the Tapestry for the first time. Root node should be called "default node" from now on, since it reflects which node gets selected when viewing the tapestry for the first time.
  • Allow deleting any node and any link.
  • Whenever a node is added to an empty tapestry, that node will become the default node. If the default node has been changed to a node different than the first node added (which is not possible with this PR - but this PR opens up the possibility for a future PR to implement the ability for authors to set a node as the default node), and the default node is deleted, then the node selected for a first-time viewer of the tapestry will fall back to the first node in the tapestry by the order of creation until otherwise overwritten by setting an explicit rootId value.
  • Allow any user to create a new node on an empty tapestry (for non-admins, they will have to submit the node for review).

New draft node behaviours

  • Administrators should be able to directly publish any draft node, including a standalone draft node, and a draft node that is only attached to other draft nodes.
  • Updated draft* node permissions for author / reviewer (*Note: accepted draft nodes are PUBLISHED nodes, so they are not considered as draft nodes here):
    • If node is not submitted for review:
      • Allow all actions except "add" for original author
      • Allow all actions except "edit" for reviewer if node is rejected and showRejected is true
    • If node is submitted for review:
      • Only allow "read" for original author
      • Allow all actions except "edit" for reviewer
  • Users can now create a standalone draft node via a "hack": first they can add a draft child node to a published node, then they can remove the link from the published node to the draft node making their draft node standalone. [need to verify if this is expected behaviour]

Bug fixes and improvements

  • Bug fix: the minimap is showing even when the Tapestry is empty.
  • Bug fix: links associated with a node are not being deleted in the backend when the node is deleted. This is a small but significant bug due to an associative array not being cast to a PHP object. The bug does not affect any visuals because links to nonexistent nodes are filtered out by the backend & frontend - it is also the reason why this bug was not found until this PR.
  • Improvement: the error message and status code of a TapestryError in the backend can be overridden by explicitly specifying them; if not overridden, the values in the TapestryError's ERRORS array will be used.

Screenshot

image

Issue Linkage

Closes #1221

Closes #1223

PR Dependency

Depends on: #1276

Automated Testing

  • N/A
  • (unsure if we need some automated testing for this)

@legendword legendword changed the base branch from 1219-tapestry-3.0 to 1190-zoomable-interface July 22, 2022 00:07
@legendword
Copy link
Contributor Author

legendword commented Jul 26, 2022

image

A bug with creating draft nodes:

  1. create a node
  2. add a draft child node to it
  3. delete the parent node
  4. submit the child node for review
  5. child node cannot be accepted since the parent node is now gone

Is this the expected behaviour?

Implemented reversible changes that allow accepting standalone draft nodes and draft child nodes of draft nodes in f8898fc

@legendword
Copy link
Contributor Author

legendword commented Jul 26, 2022

[not related to the above bug, but a side note] While debugging I noticed this piece of code in the Sidebar component, where if el is found it calls console.error. Is this a mistake?

scrollToRef(refName) {
if (refName) {
this.active = refName
this.$nextTick(() => {
let el = this.$refs[refName]
if (el.hasOwnProperty("$el")) {
el = el.$el
}
if (el) {
console.error("Cannot scroll to non-existing element ", el)
this.$refs.content.scroll(0, el.offsetTop - PADDING_OFFSET)
}
})
}
},

[update] This indeed is a mistake. Since it is a small, rather unrelated change, the fix will be merged into master as a part of #1229

@legendword
Copy link
Contributor Author

legendword commented Jul 26, 2022

[FIXED]

Bug: adding node in empty tapestry results in a link to itself (happens after deleting all existing nodes and adding a first node again)

image

@legendword
Copy link
Contributor Author

legendword commented Jul 26, 2022

Bug: sometimes an extra "updateNode" is triggered with the ID of an already deleted node when adding a node in an empty tapestry after deleting all nodes, which causes the backend to throw NodeMetaId invalid error. I have yet to be able to consistently reproduce this bug but it is a critical one that needs fixing.

[update] After encountering this bug again when debugging on another branch, I realized this may be due to the Webpack hot reloading re-mounting the NodeModal which adds an "add-node" event listener to the root of the NodeModal component tree (not the NodeModal itself, and apparently Webpack does not remove the registered event listener from the root component when refreshing the NodeModal code). This results in multiple "add-node" event listeners being triggered for one "add-node" event. Refreshing the page will eliminate this problem. Since it is due to the development environment's hot reloading functionality, this bug does not affect the production environment and can be easily avoided during development, too. In any case, it is good to know that this issue exists and why it happens.

@legendword legendword added needs testing Has been tested by author and needs a second pair of eyes to test it needs first review Code has not been reviewed yet labels Jul 27, 2022
@legendword
Copy link
Contributor Author

legendword commented Jul 27, 2022

FIXED in 2de0a8e

Bug: links attached to deleted nodes are still present in the backend (can be seen within the export file of the tapestry)

@legendword legendword added needs work This PR has bugs or not complete as per its requirements and removed needs testing Has been tested by author and needs a second pair of eyes to test it needs first review Code has not been reviewed yet labels Jul 27, 2022
@legendword legendword assigned legendword and unassigned zifgu Jul 27, 2022
@legendword legendword force-pushed the 1221-standalone-nodes branch from 1e299cd to f8898fc Compare July 28, 2022 01:58
@legendword legendword added needs work This PR has bugs or not complete as per its requirements and removed needs final review Has been tested and reviewed once and needs another code review to become ready for merge labels Sep 23, 2022
@legendword legendword assigned legendword and unassigned wynnset Sep 23, 2022
@legendword legendword added needs final review Has been tested and reviewed once and needs another code review to become ready for merge and removed needs work This PR has bugs or not complete as per its requirements labels Sep 27, 2022
@legendword legendword assigned wynnset and unassigned legendword Sep 27, 2022
@cypress
Copy link

cypress bot commented Oct 21, 2022



Test summary

97 0 0 0Flakiness 0


Run details

Project tapestry-wp
Status Passed
Commit e10419d
Started Jan 4, 2023 10:15 PM
Ended Jan 4, 2023 10:19 PM
Duration 03:49 💡
OS Linux Ubuntu - 22.04
Browser Chrome 108

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@Bitajkr
Copy link

Bitajkr commented Nov 4, 2022

@legendword legendword changed the base branch from 1198-accessibility to 1265-node-info-on-lightbox November 27, 2022 01:09
@Bitajkr
Copy link

Bitajkr commented Dec 6, 2022

Bug: Sometimes I am able to create a standalone node by using the authoring toolbar's node addition button; however, sometimes this feature doesn't work. In other words, the node either won't actually stick to the page when I try to add it, or the node that ends up being created isn't clickable and keeps moving around the page.

@legendword legendword force-pushed the 1221-standalone-nodes branch from f614966 to e10419d Compare January 4, 2023 22:12
@wynnset wynnset closed this Jan 6, 2023
@wynnset wynnset deleted the 1221-standalone-nodes branch January 18, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs final review Has been tested and reviewed once and needs another code review to become ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow standalone nodes
4 participants