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

state totals fixes #4173

Merged
merged 3 commits into from
Apr 16, 2021
Merged

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Apr 15, 2021

These changes close #4157
image

Also fixes incorrect state totals (caused by only the deltas, and not the store totals, being collected).
Although I have noticed additional succeeded every now and then, which I will hunt down eventually but is only visible in TUI.

image

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 change log entry required (why? e.g. invisible to users).
  • No documentation update required.
  • No dependency changes.

@dwsutherland dwsutherland added this to the cylc-8.0b1 milestone Apr 15, 2021
@dwsutherland dwsutherland self-assigned this Apr 15, 2021
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Tested the UI, on master got the states with different values than the ones displayed in the tree view, and also some weird state for the toolbar.

With this PR, the toolbar behaves correctly, and the states in GScan and in the tree view are matching. Great job @dwsutherland !

@kinow
Copy link
Member

kinow commented Apr 15, 2021

Got the issue I had seen on master again after trying a little more. Everything else seems to work fine, and this issue appears to be related to how we update the UI state after executing a mutation. Leaving it as approved as the code looks OK and the UI is working OK (cannot tell anything is wrong with stateTotals).

Will leave it to @oliver-sanders to finish the review 👍

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

🍾

@oliver-sanders oliver-sanders merged commit dd8e26a into cylc:master Apr 16, 2021
Sign up for free to 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.

cycle point status with workflow pause
4 participants