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 627 forks source link

CampaignApiController: add missing unit tests and refactor to mediator #594

Closed mgmccarthy closed 8 years ago

mgmccarthy commented 8 years ago

Issues #567 and #59. @mikesigs just so we don't duplicate efforts

mgmccarthy commented 8 years ago

for the only action method in this controller: public IEnumerable<ActivityViewModel> GetCampaignsByPostalCode(string zip, int miles)

why are we returning a list of ActivityViewModels when the method name starts with "GetCampaigns"?

what do we really want to return here, a list of Campaigns within the allowable mileage for the given zip code? If so, then why do we need to return a list of ActivityViewModels?

var campaigns = (from c in _allReadyDataAccess.ActivitiesByPostalCode(zip, miles)
                              select c.Campaign).Distinct();

            var activities = (from c in campaigns
                              from p in c.Activities
                              select p);                           

            foreach (Activity activity in activities)
            {
                ret.Add(new ActivityViewModel(activity));
            }

            return ret;

I can't tell b/c this is an Api controller and I"m not too sure how the returned data is going to be consumed.

mgmccarthy commented 8 years ago

and another question. Why does Activity have a Campaign and CampaignId property? image

mheggeseth commented 8 years ago

@mgmccarthy This is the most common pattern for relationships in Entity Framework.

When will one be set and the other not?

If you load an Activity by itself without including its related Campaign, CampaignId will still be populated (because it's a column in the Activity table), but Campaign will be null. Campaign will be loaded if you do something like .Include(a => a.Campaign) when querying the database.

Would the Id in the Campaign returned from the Campaign property be different than the CampaignId property?

No

Defining CampaignId like this is not required, but it is convenient in cases where we want to know whether and which Campaign an Activity is related to without loading the entire Campaign record.

mgmccarthy commented 8 years ago

@mheggeseth thanks for the explanation. I currently have been working on an SOA systems with strict bounded contexts and a very simplified API over our data access that result in our entity relations being very simple, and if there is a entity that has a class as a property, that class is a value object.