pass DS.SnapshotRecordArray to build-url-mixin buildURL

[trevorrjohn]

See discussion: #4302

[sly7-7]

I think you don't have to add this parameter here. It is passed as the snapshot argument

[trevorrjohn]

OK, I was confused with this as I wasn't sure if it was an array of Snapshot or the DS.SnapshotRecordArray. Will update...

[trevorrjohn]

@pangratz I think this is more along the lines of what you were looking for.

[pangratz]

😍 awesome, that was quick! Thank you very much @trevorrjohn!

One tiny thing left: since this is a bugfix, can you prefix the commit with [BUGFIX beta] to make sure this gets into the next beta release?

[trevorrjohn]

no problem done.

[sly7-7]

Hm, I may be wrong but I think the snapshot(Array) should still be passed in buildUrl, like here:

data/addon/adapters/rest.js

Line 461 in 8959c4e

const url = this.buildURL(type.modelName, null, null, 'findAll');

[trevorrjohn]

you're right, I missed that when I was removing that other code.

[pangratz]

Awesome catch @sly7-7. Completely missed that. Looks like this has been removed accidentally when our comments were addressed. Since no tests fail here I think that's a good sign that there is one missing, which tests this scenario.

@trevorrjohn can you pass the snapshotRecordArray where @sly7-7 pointed it out and additionally add a test in tests/integration/adapter/build-url-mixin-test.js, which asserts that a snapshot array is passed to urlForFindAll? You can take this test as an inspiration.

[pangratz]

@trevorrjohn nvm, just saw the test you added. That one looks good too!

[sly7-7]

Unfortunately, I don't have the time right now, but I think there are other method where the snapshot is not passed. (here for example

data/addon/adapters/rest.js

Line 637 in 8959c4e

url = this.urlPrefix(url, this.buildURL(type, id, null, 'findHasMany'));

) I'm not sure if we should pass the snapshots where it's available, but in that case, maybe we could add some integration test to have the complete flow tested (from the store.findXXX methods).

[trevorrjohn]

Cool! Yeah I had it in the first commit, but accidentally ripped it out
when I was removing the extra param.

On Wed, Apr 6, 2016, 14:41 Clemens Müller notifications@github.com wrote:

@trevorrjohn https://github.com/trevorrjohn nvm, just saw the test you
added. That one looks good too!

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#4304 (comment)

[trevorrjohn]

I can look into adding it to the other methods as well.

On Wed, Apr 6, 2016, 14:45 Sylvain MINA notifications@github.com wrote:

Unfortunately, I don't have the time right now, but I think there are
other method where the snapshot is not passed. (here for example

data/addon/adapters/rest.js

Line 637 in 8959c4e

url = this.urlPrefix(url, this.buildURL(type, id, null, 'findHasMany'));

)
I'm not sure if we should pass the snapshots where it's available, but in
that case, maybe we could add some integration test to have the complete
flow tested (from the store.findXXX methods).

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#4304 (comment)

[pangratz]

I can look into adding it to the other methods as well.

Let's do that in a separate PR

[trevorrjohn]

OK do you want one for each method or can I do the rest in one?

On Wed, Apr 6, 2016, 14:48 Clemens Müller notifications@github.com wrote:

I can look into adding it to the other methods as well.

Let's do that in a separate PR

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#4304 (comment)

[pangratz]

Alright, so this gets tricky now, please bear with me: the tests are failing now since you added an assertion here. This assertion will fail since the snapshot array is not passed to buildURL here, where this code path is taken when the ds-improved-ajax feature is enabled.

The feature is not yet available in the beta branch, that's why simply passing the snapshot record array here would indeed make this PR green, but it would make cherry-picking the bugfix into beta complicated (we only want the code path in the beta branch, which is not behind the ds-improved-ajax feature).

So, here's what you need to do (sorry in advance 😔): pass the snapshot array here and commit this change with the message [FEATURE ds-improved-ajax] pass snapshotRecordArray to urlForFindAll.

I will add a team discussion flag here to check that I am not going insane. @trevorrjohn I am very sorry for this mess, your first contribution should NOT be that complicated. If you have any further questions ping me on slack. Thanks for sticking through! You rock!

Note: technically, if every commit in this PR should have a green CI, you would need to rebase the commits and put the feature commit before the bugfix..

---

OK do you want one for each method or can I do the rest in one?

This could be done in one PR then, but wait before the above is figured out, since I think there will be issues with the ds-improved-ajax feature for urlForFindHasMany / urlForFindBelongsTo as well. I will let you know once that path is clear, so you can work on that PR.

[pangratz]

I discussed this with @bmac and the approach of 2 commits is the way to go here. Thanks @trevorrjohn once again for tackling this and thanks @sly7-7 for your 👀 here! Much appreciated.

Regarding "passing snapshot/snapshots" to urlForFindHasMany / urlForFindBelongsTo: @trevorrjohn the approach is the same as in this PR; so first you need to create the commits for the feature and afterwards the actual bugfix.