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

fix: Prevent Nuke from opening multiple submitter dialogs #17

Merged
merged 5 commits into from
Sep 21, 2023

Conversation

jlangmann-aws
Copy link
Contributor

What was the problem/requirement? (What/Why)

Issue: Bea-15149
Previously, a user could open multiple submitter dialogs. The user should not be able to open more than 1 submitter dialog.

What was the solution? (How)

If the submitter dialog is already open, refresh the dialog properties (including job attachments).

What is the impact of this change?

Better user experience

How was this change tested?

Job Bundle Output tests were successful

Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.

Yes, ran tests. Also tested adding/removing read node dependencies and re-opening the Submitter dialog.

Required: paste the contents of job_bundle_output_tests/test-job-bundle-results.txt here

only diff was my Nuke version (I'm using 14v5)

Was this change documented?

Is this a breaking change?

This change is dependent on - aws-deadline/deadline-cloud#35

@jlangmann-aws jlangmann-aws requested a review from a team as a code owner September 11, 2023 20:49
@jlangmann-aws jlangmann-aws force-pushed the jlangm_bea15149 branch 2 times, most recently from 6d64261 to 5f6f1eb Compare September 11, 2023 21:18
f=f,
)
else:
g_submitter_dialog.refresh(auto_detected_attachments, attachments)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want render_settings to be part of this refresh as well. If we're refreshing an existing dialog, it may be necessary to move the job_bundle_callback out of this function to no longer be a closure. This is because after updating this, the callback is still a closure to the first call that created the dialog, not any of the later updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, happy to do this while I'm at it. It will also be a slightly larger change in deadline-cloud since we'll need to add refresh functions for the Shared Job Settings and the Job Specific Settings UIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this and the deadline-cloud PR so we're now passing in the render settings. It looks like the job_bundle_callback is valid as is since we're passing in the settings (which are reset if necessary) when submitting a job or saving a bundle.

Also I see that the test coverage went slightly low with python 3.10. I can merge in this PR after Ting's review #18 is complete!

@jlangmann-aws jlangmann-aws force-pushed the jlangm_bea15149 branch 2 times, most recently from 9f22fe1 to 2089356 Compare September 15, 2023 02:15
@jlangmann-aws jlangmann-aws changed the title Prevent Nuke from opening multiple submitter dialogs fix: Prevent Nuke from opening multiple submitter dialogs Sep 15, 2023
Signed-off-by: Julie Langmann <69699954+jlangmann-aws@users.noreply.github.com>
- Now passing rendering settings to refresh call

Signed-off-by: Julie Langmann <69699954+jlangmann-aws@users.noreply.github.com>
Signed-off-by: Julie Langmann <69699954+jlangmann-aws@users.noreply.github.com>
Signed-off-by: Julie Langmann <69699954+jlangmann-aws@users.noreply.github.com>
Signed-off-by: Julie Langmann <69699954+jlangmann-aws@users.noreply.github.com>
@mwiebe mwiebe merged commit ba635ae into mainline Sep 21, 2023
6 checks passed
@mwiebe mwiebe deleted the jlangm_bea15149 branch September 21, 2023 18:17
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.

5 participants