Skip to content

Conversation

@mjkkirschner
Copy link
Member

Purpose

I found that even though we started cacheing the product keys during asm product lookup - we were not actually using them to look in the registry to find the matching product - we were only using display name.

certain products no longer use their display name, and instead use their product code.

The following PR first tries to lookup the product in the cached registry data using the product name, then it uses the product key if the former returns null.

After this PR DynamoSandbox on my machine finds Revit 2022 and Revit 2021 as products with asm.

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

@mjkkirschner mjkkirschner requested a review from pinzart February 8, 2021 22:01
@mjkkirschner mjkkirschner changed the title wip use prod code when name returns null WIP use product code to lookup product install when displayName returns null Feb 8, 2021
Copy link
Contributor

@pinzart90 pinzart90 left a comment

Choose a reason for hiding this comment

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

LGTM
Unit test for this case?

add friend assembly for moq
add test
fix broken tests
add fallback for interface
@mjkkirschner mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Feb 9, 2021
@mjkkirschner
Copy link
Member Author

@pinzart90 please take another look - I think I've added a sufficient test and fixed the other tests that this broke (their mocks needed the new method added)

I also added a fallback if someone passes an interface type to this method.

@mjkkirschner
Copy link
Member Author

@pinzart90 I think i'd like to wait on merging this until the build is back up.

@mjkkirschner mjkkirschner merged commit 8831ecd into DynamoDS:master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PTAL Please Take A Look 👀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants