Use cycle point ID in the tree view structure #505

kinow · 2020-09-24T22:16:46Z

These changes are related to #504 #339

At the moment every node in the tree has an ID like kinow|five|1|foo, except the cycle points, which use just 1 (integer-type).

I implemented that way as it was the simplest approach, and because our cycle points are built from family proxies in the GraphQL query (cycle points are not types in the schema I think). @oliver-sanders found out that we didn't have the ID's while working on the mutations & api-on-the-fly integration.

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Already covered by existing tests.
Appropriate change log entry included.
No documentation update required.

…y-proxy ID. With this we are able to derive the cycle point ID from the family-proxy ID (as the cyclepoint ID is part of it).

codecov-commenter · 2020-09-24T22:18:03Z

Codecov Report

Merging #505 into master will increase coverage by 3.08%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   63.29%   66.38%   +3.08%     
==========================================
  Files          44       43       -1     
  Lines         970      949      -21     
  Branches       59       59              
==========================================
+ Hits          614      630      +16     
+ Misses        338      301      -37     
  Partials       18       18

Flag	Coverage Δ
#unittests	`66.38% <86.66%> (+3.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/graphql/queries.js	`100.00% <ø> (ø)`
src/components/cylc/tree/index.js	`93.54% <66.66%> (-6.46%)`	⬇️
src/components/cylc/tree/cylc-tree.js	`100.00% <100.00%> (ø)`
src/graphql/graphiql.js
src/mixins/toolbar.js	`100.00% <0.00%> (+30.00%)`	⬆️
src/mixins/index.js	`100.00% <0.00%> (+100.00%)`	⬆️
src/mixins/treeview.js	`100.00% <0.00%> (+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 4a6931c...6062fb7. Read the comment docs.

kinow · 2020-09-24T22:44:45Z

The TreeItem props contain the tree node. This node now uses the cycle point ID, built from another node's ID.

The CylcTree object also stores the cycle point nodes received from GraphQL deltas using the complete cycle point ID for consistency. So either using data from deltas, or the available data in the Vue virtual DOM components data, you should have the same cycle point ID with a pattern user|workflow|cyclepoint

kinow · 2020-09-24T22:47:20Z

src/components/cylc/tree/cylc-tree.js

@@ -338,16 +340,17 @@ class CylcTree {
    // NOTE: when deleting the root family, we can also remove the entire cycle point
    if (familyProxyId.endsWith('|root')) {
      // 0 has the owner, 1 has the workflow Id, 2 has the cycle point, and 3 the family name
-      const [owner, workflowId, cyclePoint] = familyProxyId.split('|')


Removed cyclePoint as I don't need that, since I'll build the full cycle point ID

src/components/cylc/tree/cylc-tree.js

…e point ID in the deltas and CylcTree

kinow · 2020-09-24T22:50:08Z

src/graphql/queries.js

+fragment CyclePointData on FamilyProxy {
+  id
+  cyclePoint
+}


A new fragment for cycle points. Notice that it is built from a FamilyProxy, so effectively the id is a FamilyProxy ID

src/components/cylc/tree/index.js

kinow · 2020-09-24T22:53:43Z

Note to self: look at the offline data and check if we need to re-generate it

src/services/mock/one/flow.cylc

@@ -100,7 +100,4 @@
            # reset stuff
            cylc unhold \
                "${CYLC_SUITE_NAME}" "sleepy.${CYLC_TASK_CYCLE_POINT}"
-            cylc reset \
-                "${CYLC_SUITE_NAME}" "retrying.${CYLC_TASK_CYCLE_POINT}" \
-                -s 'succeeded'


kinow · 2020-09-28T00:13:51Z

Unit tests green. Almost there. I fixed the workflow one, but now got a new problem. The old data was generated before we had deltas. The code used to produce the offline data used cylc client to run a query workflows {}.

But now, cylc client doesn't support a deltas subscription (although I think we can enable that with some parameter?). But even if that worked, we couldn't really use it, because the generate shell needs 1 payload returned, instead of multiple messages with the deltas.

So my idea now is to write a new query for the offline data, re-using parts of the deltas fragments. But hit a bump with https://github.com/cylc/cylc-uiserver/issues/159

kinow · 2020-09-28T01:42:26Z

Alright, ready for review. @hjoliver is away this week, so happy if any other @cylc/core volunteers to review after @oliver-sanders , or we can wait until next week 👍

kinow · 2020-09-29T02:32:53Z

Added one commit that throws an Error if the node.id doesn't have at least three parts (part1|part2|part3). It should never happen, but in case it does, the tree would have incorrect data, and likely miss elements that need to be removed.

Tested with five, families2, and complex, no browser console errors. Then added one more commit with a unit test for that function 👍

oliver-sanders

Looks good, tested as working:

hjoliver

Tested as working 👍

Still needs a change log entry @kinow ?

kinow · 2020-10-05T21:21:16Z

Tested as working +1

Still needs a change log entry @kinow ?

Ops, just added it. Thanks @hjoliver !

Add a cycle point fragment that includes the cyclepoint and the famil…

f79cc94

…y-proxy ID. With this we are able to derive the cycle point ID from the family-proxy ID (as the cyclepoint ID is part of it).

kinow self-assigned this Sep 24, 2020

kinow added this to the 0.3 milestone Sep 24, 2020

kinow commented Sep 24, 2020

View reviewed changes

src/components/cylc/tree/cylc-tree.js Outdated Show resolved Hide resolved

Build the cycle point ID using other node's IDs, and use the new cycl…

decfed3

…e point ID in the deltas and CylcTree

kinow force-pushed the cyclepoint-id branch from 3f19ffc to decfed3 Compare September 24, 2020 22:47

kinow commented Sep 24, 2020

View reviewed changes

kinow added 2 commits September 25, 2020 12:16

Fix unit tests to use new cycle point full ID

452686c

Updating workflow one, used for offline data

644cb3e

kinow force-pushed the cyclepoint-id branch from 9bcc966 to 644cb3e Compare September 25, 2020 01:05

kinow commented Sep 25, 2020

View reviewed changes

src/services/mock/one/flow.cylc

@@ -100,7 +100,4 @@

# reset stuff

cylc unhold \

"${CYLC_SUITE_NAME}" "sleepy.${CYLC_TASK_CYCLE_POINT}"

cylc reset \

"${CYLC_SUITE_NAME}" "retrying.${CYLC_TASK_CYCLE_POINT}" \

-s 'succeeded'

This comment was marked as resolved.

Sign in to view

kinow added 2 commits September 28, 2020 12:13

Fix remaining unit tests due to changes in cycle points ID's

30bc308

Use cylc set-outputs instead of cylc reset (thanks @dwsutherland!)

014e758

Re-generate the offline data, and update e2e tests

7998014

kinow marked this pull request as ready for review September 28, 2020 01:41

kinow requested review from oliver-sanders and hjoliver September 28, 2020 01:41

kinow added 2 commits September 29, 2020 12:30

Use an error if the node ID provided is not valid

890ad42

Add unit tests for new cycle point function

4d96c95

oliver-sanders approved these changes Sep 29, 2020

View reviewed changes

oliver-sanders mentioned this pull request Oct 2, 2020

mutations in the tree view #504

Merged

13 tasks

hjoliver approved these changes Oct 5, 2020

View reviewed changes

Add changelog

6062fb7

hjoliver merged commit 31bc5c9 into cylc:master Oct 5, 2020

kinow deleted the cyclepoint-id branch October 5, 2020 21:25

kinow mentioned this pull request Oct 6, 2020

[graphql] Error running query: argument of type 'NoneType' is not iterable cylc/cylc-flow#3855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cycle point ID in the tree view structure #505

Use cycle point ID in the tree view structure #505

kinow commented Sep 24, 2020 •

edited

Loading

codecov-commenter commented Sep 24, 2020 •

edited

Loading

kinow commented Sep 24, 2020

kinow Sep 24, 2020

kinow Sep 24, 2020

kinow commented Sep 24, 2020 •

edited

Loading

This comment was marked as resolved.

kinow commented Sep 28, 2020

kinow commented Sep 28, 2020

kinow commented Sep 29, 2020

oliver-sanders left a comment

hjoliver left a comment

kinow commented Oct 5, 2020

Use cycle point ID in the tree view structure #505

Use cycle point ID in the tree view structure #505

Conversation

kinow commented Sep 24, 2020 • edited Loading

codecov-commenter commented Sep 24, 2020 • edited Loading

Codecov Report

kinow commented Sep 24, 2020

kinow Sep 24, 2020

Choose a reason for hiding this comment

kinow Sep 24, 2020

Choose a reason for hiding this comment

kinow commented Sep 24, 2020 • edited Loading

This comment was marked as resolved.

kinow commented Sep 28, 2020

kinow commented Sep 28, 2020

kinow commented Sep 29, 2020

oliver-sanders left a comment

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

kinow commented Oct 5, 2020

kinow commented Sep 24, 2020 •

edited

Loading

codecov-commenter commented Sep 24, 2020 •

edited

Loading

kinow commented Sep 24, 2020 •

edited

Loading