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

Crash creating automatic backup of Custom Node with special Characters #9287

Merged
merged 17 commits into from
Dec 14, 2018

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Dec 3, 2018

Purpose

This PR is to address the crash issues that were occurring due to special or non-printable characters in the Custom Node name.
https://jira.autodesk.com/browse/QNTM-3928.

Declarations

Check these if you believe they are true

  • The code base 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.

Reviewers

@mjkkirschner @QilongTang @Racel @alfarok

FYIs

@ColinDayOrg

Error window for no category:
categoryerror

Error window for invalid file name:
invalidname

@QilongTang
Copy link
Contributor

Looks Good to me with one comment. UI interaction makes sense? @Racel

@QilongTang QilongTang self-assigned this Dec 4, 2018
@Racel
Copy link
Contributor

Racel commented Dec 5, 2018

@reddyashish UI looks good. Please add tests.

@reddyashish
Copy link
Contributor Author

Added the tests.

@QilongTang
Copy link
Contributor

@reddyashish Can you do one favor for me and check if there is any file just for testing utilities? Because the place you put the test since does not seem to be the best, the tests there all tests serialization function which I think the test you end up using does not involve serialization

@QilongTang
Copy link
Contributor

@reddyashish Only that last comment, otherwise, LGTM

@reddyashish
Copy link
Contributor Author

reddyashish commented Dec 13, 2018

@QilongTang I have moved the test to the NodeViewCustomizationTests.cs file. There is a UtilityTests.cs test suite as well but this looked more appropriate to me, as this had tests checking other functions of PathHelper class. Let me know if you think otherwise.

@QilongTang
Copy link
Contributor

QilongTang commented Dec 13, 2018

@reddyashish NodeViewCustomization does not seem proper either, can you move it to Dynamo\test\DynamoCoreTests\UtilityTests.cs? The nodeViewCustomization is for testing a customized NodeModel nodes with radio buttons, drops a 3rd party developer customize.

@QilongTang
Copy link
Contributor

@reddyashish LGTM, once the PR checks passed, feel free to merge. Good work!

@QilongTang QilongTang added LGTM Looks good to me bug labels Dec 13, 2018
@QilongTang QilongTang added the 2.x Issues related to 2.x versions of Dynamo. label Dec 13, 2018
@reddyashish
Copy link
Contributor Author

Yes. I will check that and merge it.

@reddyashish reddyashish merged commit 8589ac4 into DynamoDS:master Dec 14, 2018
@horatiubota horatiubota added the error/warning/crash Issues mentioning a Dynamo error, warning or crash message label Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Issues related to 2.x versions of Dynamo. bug error/warning/crash Issues mentioning a Dynamo error, warning or crash message LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants