Skip to content

Xtriggers and task run mode in info view #2102

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Mar 12, 2025

Requires cylc/cylc-flow#6671 [MERGED]

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).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this Mar 12, 2025
@wxtim

This comment was marked as resolved.

@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from a7d0e0a to 8e4dc72 Compare March 18, 2025 10:33
@wxtim wxtim marked this pull request as draft March 18, 2025 10:43
@wxtim wxtim mentioned this pull request Mar 31, 2025
18 tasks
@wxtim
Copy link
Member Author

wxtim commented Apr 8, 2025

Note to self - remove the note from cylc doc added in cylc/cylc-doc#816 about info view not showing xtriggers.

@wxtim wxtim marked this pull request as ready for review May 1, 2025 07:35
@wxtim
Copy link
Member Author

wxtim commented May 1, 2025

PR is ready for review, but only works in combination with cylc/cylc-flow#6671

@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from 077f8cd to 73a93e2 Compare May 1, 2025 07:51
@wxtim

This comment was marked as resolved.

@MetRonnie

This comment was marked as resolved.

@wxtim wxtim marked this pull request as draft May 13, 2025 09:31
@wxtim

This comment was marked as resolved.

@MetRonnie

This comment was marked as resolved.

@wxtim

This comment was marked as resolved.

@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from 73a93e2 to 0356607 Compare May 16, 2025 15:22
@wxtim wxtim marked this pull request as ready for review May 19, 2025 07:07
@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from 0356607 to b5be56b Compare May 19, 2025 07:20
@oliver-sanders oliver-sanders requested review from hjoliver and removed request for MetRonnie May 21, 2025 08:48
@hjoliver
Copy link
Member

(I've started looking at this one)

@oliver-sanders
Copy link
Member

oliver-sanders commented May 23, 2025

(I've started looking at this one)

For context, task filtering based on xtrigger-derived state & task icon badges will follow in future PRs, this one is just about the info view.

@oliver-sanders oliver-sanders added this to the 2.8.0 milestone Jun 10, 2025
@oliver-sanders
Copy link
Member

Something is going wrong with the trigger time field:

Screenshot from 2025-06-10 13-00-40

And what's up with the sort order? Why is the latest xtrigger in the middle?

@oliver-sanders
Copy link
Member

LGTM, small issue with xtrigger sorting and trigger time calculation.

Note, we could just replace the trigger_time field in the xtriggers signature with the ISO8601 value to avoid adding an extra column (that only applies to @clock triggers).

@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from d741ed5 to 6c5f082 Compare June 18, 2025 12:27
@oliver-sanders oliver-sanders changed the title Xtriggers and mode in info view Xtriggers and task run mode in info view Jun 18, 2025
@wxtim
Copy link
Member Author

wxtim commented Jun 19, 2025

Something is going wrong with the trigger time field:

I've removed seconds. Forgot about retry triggers when I did that. Thought that minutes would be fine.

And what's up with the sort order? Why is the latest xtrigger in the middle?

I've done nothing to sort the items. I imagine accidentally shearing seconds off them won't help. It seems to have sorted itself out in the process of fixing the other issues.

@oliver-sanders
Copy link
Member

oliver-sanders commented Jun 20, 2025

The re-formatted trigger-time field looks great.

The xtriggers are still being displayed in the order the backend provides them in which is unsorted. Not a biggie, but I think it would be good for the info view to sort by label, then signature.

Screenshot from 2025-06-20 10-34-40

Screenshot from 2025-06-20 10-43-08

@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from c977b7c to 97c4037 Compare June 20, 2025 13:11
Show wall_clock Trigger time in Z time.

* Use v-table for xtriggers
* Get rid of the trigger time column and modify the trigger_time to ISO in place
* Remove unwanted css

add sorting for xtriggers

update test to reflect sort
@wxtim wxtim force-pushed the feat.xtriggers_and_mode_info_view branch from 97c4037 to 6478c48 Compare June 20, 2025 13:26
@oliver-sanders oliver-sanders removed the request for review from hjoliver June 25, 2025 14:49
</tr>
</thead>
<tbody>
<tr v-for="xt in xtriggers" :key="xt">
Copy link
Member

Choose a reason for hiding this comment

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

key is supposed to be a primitive like string or number - https://vuejs.org/api/built-in-special-attributes.html#key

Suggested change
<tr v-for="xt in xtriggers" :key="xt">
<tr v-for="xt in xtriggers" :key="xt.id">

Comment on lines +288 to +294
const xtriggers = this.task?.node?.xtriggers
xtriggers.forEach(xtrigger => {
if (xtrigger.satisfied === true) {
xtrigger.satisfactionIcon = mdiCheckboxOutline
} else {
xtrigger.satisfactionIcon = mdiCheckboxBlankOutline
}
Copy link
Member

Choose a reason for hiding this comment

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

This is mutating the task prop, which should be avoided as it could lead to confusing behaviour.

Edit: seeing as I'm looking at this, I've addressed it quickly: wxtim#7

Comment on lines +58 to +74
.get('.c-info .v-expansion-panel:nth-child(4)')
.find('button')
.click({ force: true })
.get('.v-expansion-panel--active')
.should('have.length', 4)

.get('.c-info .v-expansion-panel:nth-child(5)')
.find('button')
.click({ force: true })
.get('.v-expansion-panel--active')
.should('have.length', 5)

.get('.c-info .v-expansion-panel:nth-child(6)')
.find('button')
.click({ force: true })
.get('.v-expansion-panel--active')
.should('have.length', 6)
Copy link
Member

Choose a reason for hiding this comment

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

What's the use in testing the same mechanism repeatedly? Better to test that clicking the expansion panel titled "prerequisites" leads to a prerequisite task being visible and so on?

Edit: #2102 (comment) was marked as resolved but is not?

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.

4 participants