Skip to content

Conversation

@StudioLE
Copy link
Contributor

Purpose

JIRA: DYN-1078

Issue: #8741

Prevent the user from being able to remove default ports using the - button on DSVarArgFunction nodes

Behaviour before:

DYN-1078-Remove-Default-Inputs-Before-v2

Behaviour after:

DYN-1078-Remove-Default-Inputs-Completed

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@mjkkirschner @ QilongTang @mmisol

FYIs

@Amoursol

@mjkkirschner
Copy link
Member

@StudioLE - thanks for the fix- can you also try adding a test that calls the RemoveInputFromModel method and asserts the number of ports is still equal to the default?

@mjkkirschner
Copy link
Member

@StudioLE - unfortunately there are 4 failing tests now:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1861/

@SHKnudsen should have access to this dashboard if you do not -

can you try these out locally?

@SHKnudsen
Copy link
Contributor

Thanks @mjkkirschner, we'll have a look.

@StudioLE
Copy link
Contributor Author

StudioLE commented Dec 3, 2020

Thanks @mjkkirschner. This is now resolved with 70f0e24.

image

@mjkkirschner mjkkirschner self-requested a review December 4, 2020 00:28
@QilongTang QilongTang merged commit 49d69ba into DynamoDS:master Dec 4, 2020
model.InPorts.Remove(port);
}

MarkNodeDirty();
Copy link
Contributor

Choose a reason for hiding this comment

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

@StudioLE @mjkkirschner One thing I found from my local testing of this is that, even with the default state and user would not delete default ports, but node still marked dirty which I think is not necessary because that will trigger a graph run. Can we make another PR and put the statement in the if above?

@QilongTang QilongTang added this to the 2.10.0 milestone Dec 4, 2020
QilongTang pushed a commit that referenced this pull request Dec 4, 2020
* Only remove inputs when count is greater than defaultNumInputs

* Only remove inputs when count is greater than defaultNumInputs

* Added test to ensure default ports can't be removed

* Don't remove default inputs for DSVarArgFunction nodes
QilongTang added a commit that referenced this pull request Dec 4, 2020
* Only remove inputs when count is greater than defaultNumInputs

* Only remove inputs when count is greater than defaultNumInputs

* Added test to ensure default ports can't be removed

* Don't remove default inputs for DSVarArgFunction nodes

Co-authored-by: Laurence Elsdon <laurence.elsdon@matterlab.co>
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