Fix updating task states, preventing state="" replacing existing state #586

kinow · 2021-01-27T22:32:31Z

This is a small change with no associated Issue.

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).
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
No documentation update required.

kinow · 2021-01-27T23:11:17Z

This should be fixed now. The test failures are fixed in #584

kinow · 2021-01-27T23:11:56Z

@hjoliver we can wait for #584 to be merged before reviewing. Then I'll rebase this one and CI will be OK again.

hjoliver · 2021-01-27T23:47:17Z

Just tested, the unknown task status is gone, but now the held indicator doesn't show up.

kinow · 2021-01-27T23:48:24Z

Just tested, the unknown task status is gone, but now the held indicator doesn't show up.

Hmmm. @hjoliver what happens if you hold a task, and then expand it in the tree?

hjoliver · 2021-01-27T23:52:33Z

Then it shows up!

kinow · 2021-01-28T01:33:48Z

Rebased 🤞

codecov-io · 2021-01-28T01:42:47Z

Codecov Report

Merging #586 (a1e5b82) into master (a5c7750) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
- Coverage   81.66%   81.59%   -0.08%     
==========================================
  Files          66       66              
  Lines        1320     1320              
  Branches       81       81              
==========================================
- Hits         1078     1077       -1     
- Misses        223      224       +1     
  Partials       19       19

Flag	Coverage Δ
e2e	`48.63% <100.00%> (-0.08%)`	⬇️
unittests	`79.79% <50.00%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
src/components/cylc/tree/TreeItem.vue	`96.15% <ø> (ø)`
src/components/cylc/tree/index.js	`100.00% <ø> (ø)`
src/graphql/queries.js	`100.00% <ø> (ø)`
src/components/cylc/tree/cylc-tree.js	`99.31% <100.00%> (-0.69%)`	⬇️

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 a5c7750...a1e5b82. Read the comment docs.

hjoliver · 2021-01-28T04:39:28Z

From @kinow on Element chat:

looks like it's just a reactivity issue ...

I've done some progress with the Task component not updating. When I update name or status, the new values are immediately rendered (both are string 🤔).

But changing progress (number) or isHeld (Boolean) has no effect until something else changes (like status, expanded in another element, etc) and then the component re-renders

kinow · 2021-01-28T21:25:32Z

Rebased

kinow · 2021-01-28T22:38:47Z

@hjoliver I was adding the ID to the element key, so I could more easily manipulate/find it in vue-dev-tools or programmatically while debugging the issue.

But adding :key="node.node.id" appears to have made it work 🕵️ . My guess is that Vue was not able to link the change in state to the component. But having the :key of the component, when the data changed Vue was able to now locate the component and update it. But I would like to keep digging in the reactivity of the CylcTree and components using it a bit longer after this issue.

But if that fixes the issue for now, at least we can fix this issue and open another one for follow-up.

kinow · 2021-01-28T22:45:04Z

To test, I have Chromium and Firefox open, in two monitors. With queued workflow (how I named the workflow you sent me yesterday @hjoliver). After a hard-refresh with CTRL + F5, things appear to be working OK. Though some indication that the UI is working after/during a mutation is executed would be helpful too.

In Firefox I hold a task, then confirm Chromium and Firefox updated the component. Then do the same with Chromium, confirming in Firefox. Some times I hold a task, some times release. Removing and killing appears to work fine too.

hjoliver · 2021-01-29T03:11:24Z

Hmmm, isHeld still isn't reactive for me (updated branch, re-install, re-build, hard refresh). Tried with Firefox and Chrome.

kinow · 2021-02-08T19:24:04Z

I think I found the issue. Appears to be because isHeld is not defined in the deltas.added, due to stripNull: true (when stripNull: false, isHeld is returned as false for the task proxies).

When that happens, the JS object added to the CylcTree does not have a isHeld attribute, so Vue doesn't create an observer for it. When we receive a delta with the update that contains isHeld: any-value, we merge object, setting isHeld in the object.

But since there's no observer for isHeld, Vue components do not react. I tested changing locally added(stripNull: false) and the component was updated correctly. (@hjoliver I will leave my last commit from some days ago, key-ing the components, as that's actually good; and when vue-dev utils is open, that appears to somehow trigger the component update, probably that's why it was working for me but didn't work for you).

Posted a question to the Element chat room to see if stripNull is working as expected. A possible solution for this is simply use added(stripNull: false), but will wait for the reply there first.

kinow · 2021-02-08T19:40:29Z

kinow · 2021-02-08T20:19:29Z

@hjoliver whenever you have time, could you please take another look and confirm if that fixes the issue for you? The isHeld should be applied correctly now after you trigger the mutation 🤞 Thanks!

src/graphql/queries.js

oliver-sanders

This seems like a logical change to me though I've encountered the issue it's fixing myself. Hopefully Hillary can confirm it fixes the reported issue.

kinow

I've moved it back to draft as I've incorporated these changes into #530 since the state could be interfering with the outputs being updated correctly.

If that PR is merged, we can probably close this one as superseded. I will take a better look after #530 is merged.

kinow · 2021-02-17T20:57:20Z

src/graphql/queries.js

+  # value for isHeld, and then it is stripped. When stripNull: false is used,
+  # we will actually get the default value from Protobuf (in the case of
+  # isHeld, we will get isHeld: false, which is what we want for the UI).
+  added (stripNull: false) {


Marking as draft. @hjoliver the new merge customizer (from Lodash API) used in #530 appears to remove the need for this stripNull. Will test again once we have merged that PR.

kinow · 2021-02-17T20:57:45Z

tests/unit/components/cylc/tree/tree.spec.js

+      })
+      cylcTree.updateTaskProxy(updateTaskProxy)
+      expect(cylcTree.root.children[0].children[0].children[0].node.state).to.equal(TaskState.RUNNING.name)
+    })


Added in #530

kinow · 2021-02-17T20:58:03Z

tests/unit/components/cylc/tree/index.spec.js

-    delete taskProxy.state
-    const taskProxyNode = createTaskProxyNode(taskProxy)
-    expect(taskProxyNode.node.state).to.equal('')
-  })


Removed in #530

kinow · 2021-02-24T19:23:51Z

Confirmed it has been added to #530, with the only difference being that we did not need the stripNull: true in the added delta. @hjoliver I've tested the UI after updating my master branch, everything seems to be working OK.

Calling mutations with the aotf form, then the state is updated correctly in the UI in ~1 second. Without the need of changing the expand/collapse state of the component.

kinow self-assigned this Jan 27, 2021

kinow added this to the 0.3 milestone Jan 27, 2021

kinow requested review from hjoliver and oliver-sanders January 27, 2021 23:11

kinow force-pushed the fix-task-state-queued branch from e1e90a9 to 5fc43ba Compare January 28, 2021 01:32

kinow added 3 commits January 29, 2021 10:25

Fix updating task states, preventing state="" replacing existing state

1534d0f

Remove old unit test case, add a new one to check merging statuses

bc88b4e

Changelog

635a2c6

kinow force-pushed the fix-task-state-queued branch from 5fc43ba to 635a2c6 Compare January 28, 2021 21:25

Key-ify each component linking with the node ID

45c606b

kinow mentioned this pull request Feb 8, 2021

Deltas: warn when we are working without reactivity #589

Closed

Set stripNull: false, with a comment why

a1e5b82

kinow mentioned this pull request Feb 8, 2021

GH actions: macos build failing intermitently #590

Closed

This was referenced Feb 8, 2021

GraphiQL: Document what stripNull does cylc/cylc-flow#4069

Merged

Use Lodash's mergeWith and warn when losing reactivity #591

Closed

oliver-sanders reviewed Feb 11, 2021

View reviewed changes

src/graphql/queries.js Show resolved Hide resolved

oliver-sanders approved these changes Feb 11, 2021

View reviewed changes

kinow mentioned this pull request Feb 16, 2021

Display message triggers #530

Merged

12 tasks

kinow marked this pull request as draft February 17, 2021 20:56

kinow commented Feb 17, 2021

View reviewed changes

kinow closed this Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix updating task states, preventing state="" replacing existing state #586

Fix updating task states, preventing state="" replacing existing state #586

kinow commented Jan 27, 2021 •

edited

Loading

kinow commented Jan 27, 2021

kinow commented Jan 27, 2021

hjoliver commented Jan 27, 2021

kinow commented Jan 27, 2021

hjoliver commented Jan 27, 2021

kinow commented Jan 28, 2021

codecov-io commented Jan 28, 2021 •

edited

Loading

hjoliver commented Jan 28, 2021

kinow commented Jan 28, 2021

kinow commented Jan 28, 2021 •

edited

Loading

kinow commented Jan 28, 2021 •

edited

Loading

hjoliver commented Jan 29, 2021

kinow commented Feb 8, 2021

kinow commented Feb 8, 2021

kinow commented Feb 8, 2021

oliver-sanders left a comment •

edited

Loading

kinow left a comment

kinow Feb 17, 2021

kinow Feb 17, 2021

kinow Feb 17, 2021

kinow commented Feb 24, 2021

Fix updating task states, preventing state="" replacing existing state #586

Fix updating task states, preventing state="" replacing existing state #586

Conversation

kinow commented Jan 27, 2021 • edited Loading

kinow commented Jan 27, 2021

kinow commented Jan 27, 2021

hjoliver commented Jan 27, 2021

kinow commented Jan 27, 2021

hjoliver commented Jan 27, 2021

kinow commented Jan 28, 2021

codecov-io commented Jan 28, 2021 • edited Loading

Codecov Report

hjoliver commented Jan 28, 2021

kinow commented Jan 28, 2021

kinow commented Jan 28, 2021 • edited Loading

kinow commented Jan 28, 2021 • edited Loading

hjoliver commented Jan 29, 2021

kinow commented Feb 8, 2021

kinow commented Feb 8, 2021

kinow commented Feb 8, 2021

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

kinow Feb 17, 2021

Choose a reason for hiding this comment

kinow Feb 17, 2021

Choose a reason for hiding this comment

kinow Feb 17, 2021

Choose a reason for hiding this comment

kinow commented Feb 24, 2021

kinow commented Jan 27, 2021 •

edited

Loading

codecov-io commented Jan 28, 2021 •

edited

Loading

kinow commented Jan 28, 2021 •

edited

Loading

kinow commented Jan 28, 2021 •

edited

Loading

oliver-sanders left a comment •

edited

Loading