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 a few things regarding choices and actions #1365

Merged
merged 1 commit into from
Feb 6, 2019

Conversation

v-kydela
Copy link
Contributor

@v-kydela v-kydela commented Feb 6, 2019

  • Modify ChoiceFactory.HeroCard to incorporate each choice's Action property the way ChoiceFactory.SuggestedAction does
  • Centralize that functionality in a shared method
  • Update the Choice.Action doc comment to reflect that
  • Add tests in ChoiceFactoryTests to account for actions in hero cards and suggested actions
  • Add basic test for ChoiceFactory.HeroCard that was missing

This is meant to piggyback on garypretty's recent PR since I noticed Choice.Action wasn't incorporated into ChoiceFactory.HeroCard: #1339

- Modify ChoiceFactory.HeroCard to incorporate each choice's Action property the way ChoiceFactory.SuggestedAction does
- Centralize that functionality in a shared method
- Update the Choice.Action doc comment to reflect that
- Add tests in ChoiceFactoryTests to account for actions in hero cards and suggested actions
- Add basic test for ChoiceFactory.HeroCard that was missing
@v-kydela
Copy link
Contributor Author

v-kydela commented Feb 6, 2019

I want to note that at some point I think ChoiceFactoryTests should be updated with more tests to account for every channel. This could be made easier if some private methods were included to account for the code that keeps getting repeated across the tests. I also notice that the JS repo has some choice factory tests that this repo does not.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 48584

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 72.176%

Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder.Dialogs/Choices/ChoiceFactory.cs 7 79.7%
Totals Coverage Status
Change from base Build 48579: 0.01%
Covered Lines: 3738
Relevant Lines: 5179

💛 - Coveralls

@cleemullins cleemullins merged commit ea1f8e1 into master Feb 6, 2019
@cleemullins cleemullins deleted the v-kydela/ChoiceActions branch February 6, 2019 19:47
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.

None yet

3 participants