Allow user to set the order of cycle points #658

kinow · 2021-04-30T02:48:37Z

These changes close #333

A draft for discussion I think. @oliver-sanders , @hjoliver I've added a setting under the User Profile page. If you change that setting, then reload the tree view (or just navigate after you've set the value), then it should be displayed with the correct order.

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.

codecov-commenter · 2021-04-30T02:50:14Z

Codecov Report

Merging #658 (e9458ab) into master (5d9e38c) will decrease coverage by 0.15%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #658      +/-   ##
==========================================
- Coverage   85.38%   85.22%   -0.16%     
==========================================
  Files          70       70              
  Lines        1498     1509      +11     
  Branches      113      116       +3     
==========================================
+ Hits         1279     1286       +7     
- Misses        187      188       +1     
- Partials       32       35       +3

Flag	Coverage Δ
e2e	`68.12% <58.33%> (-0.10%)`	⬇️
unittests	`76.49% <100.00%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
src/views/Tree.vue	`83.33% <0.00%> (-4.91%)`	⬇️
src/views/Workflow.vue	`88.88% <0.00%> (-1.68%)`	⬇️
src/views/UserProfile.vue	`66.66% <50.00%> (-13.34%)`	⬇️
src/components/cylc/tree/cylc-tree.js	`99.36% <100.00%> (+0.02%)`	⬆️

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 5d9e38c...e9458ab. Read the comment docs.

src/views/UserProfile.vue

hjoliver · 2021-05-03T00:07:53Z

Works well.

Seems to me this is fine as a user profile preference. It's likely to be a static choice, not something people want to change for different workflows. Also, if we keep it there, we don't have to worry about reversing existing open tree views, because there won't be any.

On naming, maybe "Latest cycle point at top", or if we want top to be the default, "Latest cycle point at bottom".

kinow · 2021-05-04T01:07:58Z

Reverted change to spelling of colour, and set the user profile settings name to "Latest cycle point at top". Will let @oliver-sanders have time to look at this draft before adding tests/changelog and marking it ready for review.

hjoliver · 2021-05-20T03:40:26Z

Still Draft? (I guess you want @oliver-sanders opinion on the questions above?)

kinow · 2021-05-20T03:53:38Z

Still Draft? (I guess you want @oliver-sanders opinion on the questions above?)

That's right. If he agrees with the current implementation I will write the tests and changelog.

oliver-sanders · 2021-05-20T08:45:25Z

That's right. If he agrees with the current implementation I will write the tests and changelog.

Sorry, missed this.

where should the setting for the tree view be set? In the user preference

Sounds good to me, I can't think why users would want this to be different in one view to another.

or we can leave it so that users have to refresh browser

That sounds fine, a notice saying "reload this page for changes to take effect" (or something like that) should be enough. We can always come back to this later if we accumulate other similar settings.

kinow · 2021-05-23T20:12:21Z

Adding test and preparing to mark this PR as ready for review today/tomorrow 👍

kinow · 2021-05-23T20:16:01Z

Rebased first.

kinow · 2021-05-24T01:56:52Z

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

@@ -186,6 +186,43 @@ describe('CylcTree', () => {
      expect(cylcTree.root.children[0].id).to.equal(`${WORKFLOW_ID}|2`)
      expect(cylcTree.root.children[1].id).to.equal(`${WORKFLOW_ID}|1`)
    })
+    it('Should add cycle points sorted DESC by default', () => {


These two tests assume we have the DESC order by default for cycle points. Should we change it later, we just update the constant in CylcTree, and these two tests, plus the remaining tests that are expecting elements in order.

hjoliver

The reverse order (latest point at bottom) doesn't seem to be working properly.

hjoliver · 2021-05-24T21:53:16Z

kinow · 2021-05-24T22:47:10Z

Ah, debugging it for a while, finally found what's wrong! Before, I reversed the array to find the next position to add the element. But in ascending mode (1 to 2, to 3, etc) I don't need to reverse it (otherwise we get a very confusing result as you found out @hjoliver 😅 ). Trying to come up with a unit test to verify this scenario.

Thanks @hjoliver ! 💯

…urning the wrong order, add unit tests

kinow · 2021-05-24T22:57:54Z

Fixed! 🤞 Wrote a unit test that failed, then fixed the code and the test passed.

hjoliver · 2021-05-24T23:44:36Z

Ah, debugging it for a while, finally found what's wrong! Before, I reversed the array to find the next position to add the element. But in ascending mode (1 to 2, to 3, etc) I don't need to reverse it (otherwise we get a very confusing result as you found out @hjoliver sweat_smile ). Trying to come up with a unit test to verify this scenario.

Thanks @hjoliver ! 100

Yes I figured that out too and was trying to fix it for a PR to your branch ... but my JS is too slow so you bet me to it!

hjoliver · 2021-05-25T00:09:36Z

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

-      // reverse to put cyclepoints in ascending order (i.e. 1, 2, 3)
-      const cyclePoints = [...parent.children].reverse()
+      // when DESC mode, reverse to put cyclepoints in ascending order (i.e. 1, 2, 3)
+      const cyclePoints = this.options.cyclePointsOrderDesc ? [...parent.children].reverse() : parent.children


Nice, I was using another if ... else block, which was annoying hell out of me.

hjoliver

That's got it 👍

kinow self-assigned this Apr 30, 2021

kinow requested review from oliver-sanders and hjoliver and removed request for oliver-sanders April 30, 2021 02:49

kinow modified the milestones: 0.4.0, 0.5.0 Apr 30, 2021

hjoliver reviewed May 3, 2021

View reviewed changes

src/views/UserProfile.vue Outdated Show resolved Hide resolved

kinow force-pushed the sort-cyclepoints branch from 0519539 to 1d2a652 Compare May 4, 2021 01:05

kinow force-pushed the sort-cyclepoints branch from d3d7b39 to 3aec9cb Compare May 4, 2021 23:24

kinow added 2 commits May 24, 2021 08:15

Allow user to set the order of cycle points

a9ce1b2

Change property label in user profile page (thanks @hjoliver)

253cd04

kinow force-pushed the sort-cyclepoints branch from 3aec9cb to 253cd04 Compare May 23, 2021 20:15

kinow added 2 commits May 24, 2021 11:07

Add missing arguments to CylcTree

cbc7086

Correct order of ASC/DESC in if, add unit tests for CylcTree

ddaa02d

kinow mentioned this pull request May 23, 2021

Fix checkpoint workflow one #624

Closed

7 tasks

Add e2e tests

5f35fec

kinow commented May 24, 2021

View reviewed changes

Add changelog

e9458ab

kinow requested review from hjoliver and oliver-sanders May 24, 2021 02:19

kinow marked this pull request as ready for review May 24, 2021 02:20

hjoliver requested changes May 24, 2021

View reviewed changes

Fix bug found by @hjoliver where asc sorting for cycle points was ret…

efe8875

…urning the wrong order, add unit tests

kinow requested a review from hjoliver May 24, 2021 22:57

hjoliver reviewed May 25, 2021

View reviewed changes

hjoliver approved these changes May 25, 2021

View reviewed changes

oliver-sanders approved these changes May 25, 2021

View reviewed changes

oliver-sanders merged commit 5c8c2e5 into cylc:master May 25, 2021

kinow deleted the sort-cyclepoints branch May 25, 2021 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to set the order of cycle points #658

Allow user to set the order of cycle points #658

kinow commented Apr 30, 2021 •

edited

Loading

codecov-commenter commented Apr 30, 2021 •

edited

Loading

hjoliver commented May 3, 2021 •

edited

Loading

kinow commented May 4, 2021

hjoliver commented May 20, 2021

kinow commented May 20, 2021

oliver-sanders commented May 20, 2021

kinow commented May 23, 2021

kinow commented May 23, 2021

kinow May 24, 2021

hjoliver left a comment

hjoliver commented May 24, 2021 •

edited

Loading

kinow commented May 24, 2021

kinow commented May 24, 2021 •

edited

Loading

hjoliver commented May 24, 2021

hjoliver May 25, 2021

hjoliver left a comment

Allow user to set the order of cycle points #658

Allow user to set the order of cycle points #658

Conversation

kinow commented Apr 30, 2021 • edited Loading

codecov-commenter commented Apr 30, 2021 • edited Loading

Codecov Report

hjoliver commented May 3, 2021 • edited Loading

kinow commented May 4, 2021

hjoliver commented May 20, 2021

kinow commented May 20, 2021

oliver-sanders commented May 20, 2021

kinow commented May 23, 2021

kinow commented May 23, 2021

kinow May 24, 2021

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented May 24, 2021 • edited Loading

kinow commented May 24, 2021

kinow commented May 24, 2021 • edited Loading

hjoliver commented May 24, 2021

hjoliver May 25, 2021

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

kinow commented Apr 30, 2021 •

edited

Loading

codecov-commenter commented Apr 30, 2021 •

edited

Loading

hjoliver commented May 3, 2021 •

edited

Loading

hjoliver commented May 24, 2021 •

edited

Loading

kinow commented May 24, 2021 •

edited

Loading