Skip to content

Fix rendering logic of results panel controls#1082

Merged
edan-bainglass merged 2 commits into
aiidalab:mainfrom
edan-bainglass:fix-status-notification-render
Jan 13, 2025
Merged

Fix rendering logic of results panel controls#1082
edan-bainglass merged 2 commits into
aiidalab:mainfrom
edan-bainglass:fix-status-notification-render

Conversation

@edan-bainglass
Copy link
Copy Markdown
Member

Thie PR fixes incorrect logic in deciding which widgets to add to a results panel that was incorrectly rendering status notification for completed workflows.

@AndresOrtegaGuerrero
Copy link
Copy Markdown
Member

could you suggeset a test for this behaviour?, or this is while the simulation runs ?

@edan-bainglass
Copy link
Copy Markdown
Member Author

could you suggeset a test for this behaviour?, or this is while the simulation runs ?

For starters, a completed job should not have a status notification. So we could test for this. Not sure how to test while running. Our workchain generation fixture is not so flexible I think. I'll add the first though.

@AndresOrtegaGuerrero
Copy link
Copy Markdown
Member

could you suggeset a test for this behaviour?, or this is while the simulation runs ?

For starters, a completed job should not have a status notification. So we could test for this. Not sure how to test while running. Our workchain generation fixture is not so flexible I think. I'll add the first though.

I meant to get this behaviour in qe app (error) , to check the logic and the response from the app you want

@edan-bainglass
Copy link
Copy Markdown
Member Author

could you suggeset a test for this behaviour?, or this is while the simulation runs ?

For starters, a completed job should not have a status notification. So we could test for this. Not sure how to test while running. Our workchain generation fixture is not so flexible I think. I'll add the first though.

I meant to get this behaviour in qe app (error) , to check the logic and the response from the app you want

Like I said, this is a monitoring feature, so we would need a running process. This is a good fixture to add. I would do this in a separate PR that handles the testing suite as a whole (planned). But at least for the static finished process, we can ensure that the status notification is empty. This would have caught this bug.

Copy link
Copy Markdown
Member

@AndresOrtegaGuerrero AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

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

LGTM!

@edan-bainglass edan-bainglass force-pushed the fix-status-notification-render branch from 30d5bbb to 3189607 Compare January 13, 2025 16:14
@edan-bainglass edan-bainglass merged commit 4e58415 into aiidalab:main Jan 13, 2025
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.

2 participants