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

Create Campaign Flow - Initial non-published state #1583 #1627

Merged
merged 12 commits into from Dec 16, 2016

Conversation

Projects
None yet
3 participants
@HamidMosalla
Collaborator

HamidMosalla commented Dec 11, 2016

Closes: #1583

@mgmccarthy

This comment has been minimized.

Show comment
Hide comment
@mgmccarthy

mgmccarthy Dec 12, 2016

Collaborator

@HamidMosalla, I'm going to have a look at this today at some point.

Collaborator

mgmccarthy commented Dec 12, 2016

@HamidMosalla, I'm going to have a look at this today at some point.

@HamidMosalla

This comment has been minimized.

Show comment
Hide comment
@HamidMosalla

HamidMosalla Dec 12, 2016

Collaborator

@mgmccarthy Thanks! looking forward to it.

Collaborator

HamidMosalla commented Dec 12, 2016

@mgmccarthy Thanks! looking forward to it.

@mgmccarthy

@HamidMosalla, AWESOME PR! Really great/clean code. It was really easy to follow and all your changes are inline with the styles and concepts of the project.

My only code review comment is to delete some un-used using statements at the top of one of the classes you added.

If you have time, can you either:

  1. add unit tests for PublishViewModelQueryHandler and PublishCampaignCommandHandler
  2. add two Issues to the project for people that want to add the unit tests for these two classes?

We're all volunteers here, so the above two are not mandatory, but could help us keep our test coverage up.

Thanks!
Mike

clean up un-used using statements, add unit tests for PublishViewMode…
…lQueryHandler and PublishCampaignCommandHandler
@HamidMosalla

This comment has been minimized.

Show comment
Hide comment
@HamidMosalla

HamidMosalla Dec 12, 2016

Collaborator

@mgmccarthy Thanks for the review, I committed the requested changes plus the unit tests.

Collaborator

HamidMosalla commented Dec 12, 2016

@mgmccarthy Thanks for the review, I committed the requested changes plus the unit tests.

@mgmccarthy

This comment has been minimized.

Show comment
Hide comment
@mgmccarthy
Collaborator

mgmccarthy commented Dec 12, 2016

@tonysurma

This comment has been minimized.

Show comment
Hide comment
@tonysurma
Member

tonysurma commented Dec 16, 2016

Thank you @HamidMosalla

@tonysurma tonysurma merged commit 95732e4 into HTBox:master Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment