search

HTBox / allReady

This repo contains the code for allReady, an open-source solution focused on increasing awareness, efficiency and impact of preparedness campaigns as they are delivered by humanitarian and disaster response organizations in local communities.
http://www.htbox.org/projects/allready
MIT License
891 stars 626 forks source link

Review and potentially refactor EditCampaignCommandHandler #1008

Open VishalMadhvani opened 8 years ago

VishalMadhvani commented 8 years ago

There were a few bugs in EditCampaignCommandHandler specifically around managing the Campaign's Primary Contact. These were fixed in PR #1006.

EditCampaignCommandHandler could use a review as there may be more bugs lurking. Having briefly looked at the test's I'm not convinced we have full coverage so a review of all tests for this handler would also be useful.

I also noticed the handler uses extension methods for updating the CampaignImpact and Location properties of the Campaign. My preference is for this logic to be contained locally within the handler class. Any thoughts on this?

We could also look at using AutoMapper instead of doing it manually.

mgmccarthy commented 8 years ago

:+1: for pulling datataccess code out of extension methods. Extension methods should not be doing that kind of work, and if the code is used in only one place (the handler), it makes sense to pull it into the handler. There are other extension methods that could be wiped out this way,

I'm currently working on getting moving the code out of EventViewModelExtension.WithUserInfo into ShowEventQueryHandler (which is the only place that uses it)

:-1: on using AutoMapper. It's adding another framework, learning curve.... etc. Plus, I'd rather see what is happening in the actual mapping than not see it, even if that mapping is mundane. I've worked with it in the past, and it has was more trouble than it was worth. That being said, I'd defer to the project owners on if they want to include in or not

stevejgordon commented 8 years ago

@VishalMadhvani @mgmccarthy I would 100% agree with pulling the code into the handlers and making each one responsible to update all related records as necessary. The extensions just seem to make things more complicated.

I'm torn on AutoMapper. I've used it once and thought it was somewhat useful, but I'm not too concerned with having to explicitly set the properties directly. It does make it clearer what is being changed.

I came across this extension again today and I'm starting to wonder if the way contacts should work needs a discussion in itself. I'd have thought that it would be better to be able to manage a list of contacts for an org, rather than setting a contact on the main form.

@tonysurma @OhMcGoo Do you have a view on this? As far as I can tell we really only allow a single contact for an org as the UI stands currently. Would have the option to add more than one contact be useful? We could still have the ability to mark one contact as the primary. I may be missing something but the database schema seems to suggest one org can have many contacts, but in the UI this doesn't seem to be exposed. I'm happy to flesh out the idea a little further if this is useful?

tonysurma commented 8 years ago

I will defer to @OhMcGoo I think that could be helpful over time but no hard requirement just yet.

as for AutoMapper @MisterJames @BillWagner interested in your thoughts. I am have happily used it in many other apps but have no strong opinion on value of that for this use case