Fix issue with user dropdown not being populated sometimes

[begedin]

What's in this PR?

I was able to, on rare occassions, reproduce this issue on dev, but it's really difficult due to latency differences compared to staging and production.

The gist of it is, there was an issue with the organizationMembers computed defined on the organization model.

The project.tasks.index controller was returning the following in the model hook:

model() {
  let project = this.modelFor('project');
  let members = get(project, 'organization.organizationMembers');
  return RSVP.hash({ project, members });
}

At the time of the hook being executed, more often than not, the items within organizationMembers are not fully loaded. The array itself is not a promise, the individual items are promises, so the hook does not wait for them to load. Instead, we end up with most of these being set to null and this is stored as a controller property called members.

As I previously mentioned, computed will not reevaluate themselves, unless they are bound to a template. There was no direct bounding here. We get a computed from the model and evaluate it in the model hook, then store and bind to that value as a controller property.

Even if computeds did reevaluate themselves when not bound to a template, I don't think that would happen here, since we literally used get to retrieve and later store the value at a certain moment.

The solution was to alias the computed in the controller instead of evaluating it in the model hook.

Further more, I would prefer if we used computeds to facilitate template binding, not as a sort of viewModel/decorator for a model. It complicates things and in this case, also makes it quite difficult to test.

For example, I have no idea how to write a failing test for this. It would have to be acceptance level and even so, I would have to use timing and manual pushing into the store to achieve it. I tried several different things, with no luck.

References

Fixes #1082

[joshsmith]

Can you explain what the alternative is for when we need to do something like this instead of using computed properties?

[joshsmith]

@begedin when you have a chance would like a follow-up answer on the question above.

[begedin]

Using the computed property on itself is not a problem. The problem is, defining it somewhere at the bottom level, like the model, where it's not used directly, has a tendency to lead to issues like this one - instead of binding to that computed property, directly or indirectly, we ended up evaluating it in a controller hook.

Personally, I would try to avoid defining computed properties at any level other than controller/component - meaning something that's then either bound directly to a UI element, or to a component element.

The fix I did here is just a fix, I did not fully commit to what I would do. The next thing I would do is to remove the computed property on the model and instead define it in the controller using it.

In the example we had here, here's what happened:

model() {
  let project = this.modelFor('project');
  let members = get(project, 'organization.organizationMembers');
  return RSVP.hash({ project, members });
}

members is not a promise. It's a computed property. When getgot executed, it got the value of that property at that time. That value is now part of the model and in setupController is assigned to the controller. We did not assign a reference, we assigned a value.

On the other hand, if we just do

model() {
  return this.modelFor('project');
}

Now we just have a model property in the controller. If we now do the following in the controller:

members: alias('model.organization.organizationMembers')

Then this is not a value, this is a reference, so it will be evaluated correctly.

However, what I would do, is remove the organization.OrganizationMembers computed property from the organization model altogether. Instead, in the controller, I would just do:

members: mapBy('model.organization.organizationMemberships', 'member')

This is where we're using it, this is where it should be. That's the intended usage of a computed.

If we're using the same exact thing somewhere else, we define it there as well. Easier to test, easier to deal with.