Add explicit reloading of project upon donation

[begedin]

What's in this PR?

Explicitly reloads the project before transitioning to the thank you route. Also adds test assertion to ensure this happens.

References

Fixes #812

[joshsmith]

@begedin do you believe that we should not just be reloading the project when hitting its index route regardless?

[begedin]

@joshsmith In that case, we will be loading it twice when we hit the index route directly. To me, it feels like that old "wrap the whole code in a try-catch" approach.

We know we have to reload here, so this makes it explicit.

If anything, we should avoid that to and find a way to mark the project dirty, so it's background reloaded. ember-data might have such a feature.

[joshsmith]

@begedin I see the concern. My more immediate concern was that the total donated may change regularly, as might the description, its relationships, etc (people joining the project, &c). We have no real way right now to expect that to be the case when the server's data has changed but ours has not. Moving around between the tabs on the project feels like it should be fetching fresh data.

[begedin]

I can look into reloading more reliably on the project index route then. I'm not sure how all the hooks work exactly, which gets called when transitioning, which get's called when getting there directly, etc.

[joshsmith]

Okay, yeah, definitely do look into that and educate me a little on the topic, if you can. I feel like I'm still stuck in pre-Ember Data 1.0 days and I don't really understand how things work well enough right now. I would love to have a firmer grasp of what's convention now. I think we should be hitting the API sometimes more than we are (though this is not universally true, obviously).

[joshsmith]

I think my concern on this specific way of handling this is mostly a design one. I guess this makes sense as a sort of "action up", although it feels like it's a little bit of "data up". We're saying that an event happened that we think has changed something, so now we want the latest data.

I'd rather the data just flow downward from the server as it changes by just asking, not keeping track of state on the front-end. The server's state will change more often in total, I think, than our particular front-end interactions will change the server's state.

[begedin]

I looked into this, thoroughly, and I'm afraid there is no decent way to handle this in one place. Best we can do is to explicitly call project.reload() on each route for which it makes sense to do so.

I did just that in the latest commit, together with some cleaning up of things that were written weirdly. If that approach is too complex, feel free to roll it back, but I'm not sure there's a better way to do it.

We might want to consider being a bit more conservative with reloads, though, so if you feel some of these reload() calls are too much, feel free to remove those, at least.

[begedin]

I feel this makes it clearer which properties are being set.

[begedin]

project.settingsis already reloading, so I don't think project.settings.contributors has to, but I'm not sure.

[sbatson5]

🤔 This seems a bit like an anti-pattern to me. Typically, I've seen the reload method used outside of the context of the model hook, to force the model to reload because of an action (like in the example in the docs: http://emberjs.com/api/data/classes/DS.Model.html#method_reload).

Is there a reason we don't just write the model hooks we need for these child routes rather than forcing our children to trigger an update on our parent?

Rather than relying on what is returned from our parent route, can we just call the project model again here? That will update the record in our store, which will ultimately update our parent's route model as well.

Thoughts?

[joshsmith]

@sbatson5 can you clarify what you mean by calling it again?

[sbatson5]

@joshsmith I meant explicitly calling the full model hook again:

model() {
  return this.store.queryRecord('project', { ... });
}

[joshsmith]

@sbatson5 do you mean calling it from within here? Sorry I'm just not understanding where/how you would implement this specifically.

[begedin]

I took another look at this, and tried to rewrite it as a regular queryRecord. I also made use of restructuring, to make it a bit cleaner.

Here's what it looks like now:

model() {
  return this.modelFor('project').reload().then((project) => {
    let subscription = this.get('userSubscriptions').fetchForProject(project);
    return RSVP.hash({ project, subscription });
  });
}

Here's what it looks like as a regular hook

model(params) {
  return this.store.queryRecord('project', { 
    slug: params.project_slug, 
    sluggedRouteSlug: params.slugged_route_slug 
  }, { reload: true }).then((project) => {
    let subscription = this.get('userSubscriptions').fetchForProject(project);
    return RSVP.hash({ project, subscription });
  });
},

Here' what it looks like with destructuring:

model({ project_slug: slug, slugged_route_slug: sluggedRouteSlug}) {
  return this.store.queryRecord('project', { slug, sluggedRouteSlug }, { reload: true }).then((project) => {
    let subscription = this.get('userSubscriptions').fetchForProject(project);
    return RSVP.hash({ project, subscription });
  });
},

The first approach still feels like an anti-pattern to me, but it is also drier as is and the only truly clean solution would be something along the lines of server sent events anyway.

Really, we may be giving too much importance to reloading. In my opinion, the only case where we should stress about reloading the project is when the user has donated. In that specific case, they are actually expecting for donation progress to change.

In cases where a project admin has changed something about a project, like a description, it should not be that important for the regular user to see that change immediately. I think having that happen somewhere down the line, when the page refreshes or something like that, ought to be good enough, until we have server sent events, at least.

Either way, for me, it would be enough to just stick to explicit reloading when making a donation. The current implementation, where we explicitly reload on each subroute change seems like overkill to me, but I would be ok with keeping it. I would keep it as is, because the preferable solution would be server sent events, so it's drier and easier to change this way.

[joshsmith]

@begedin I think I agree with your logic here and would also like to go with @pixelhandler's approach here.

Can we simplify to whatever you feel is the best simplification for now, and open an issue to add channels for project changes?

[joshsmith]

@begedin feel free to merge without review after you decide on the best path forward, but can review in morning if needed/wanted.

[pixelhandler]

@joshsmith in the case that the server can't or won't push changes to clients, e.g. ServerSent events or a Phoenix Channel, looking for ways to poll the server for changes makes sense. The general problem with an in memory copy of a resource is that by default it's a cached representation which can become invalid aside from activity by the current user. It's a distributed cache. So Ember Data provides the 'reload' method but the app; needs to know when to do so; and since we have a SPA (singe page app) traditional full roundtrips for each page are not happening. With this PR it appears that any route below the project is simply asking (or polling) for an update, just in case there was a change.

Would it be possible to push changes from the server via a Phoenix channel instead? This would be a different implementation, i.e. routes listen for push and if the resource is used in that (listening) route it updates the resource via re-setting the model on it's associated controller.

[begedin]

I would definitely prefer the approach suggested by @pixelhandler