filter option is not handled properly for create/update

[brandonduff]

It seems like the handling of the filter option in sync is incorrect. Here's a failing test

    it "should include 'filters' from options", ->
      model = buildTimeEntry()
      model.save({}, filters: { day: 'today' })
      expect(ajaxSpy.calls.mostRecent().args[0].data).toMatch(/"day":"today"/)

The output:

1. should include 'filters' from options
   Sync updating models
   Expected '{"time_entry":{"project_id":"1"},"include":"","filters":"[object Object]","optional_fields":""}' to match /"day":"today"/.

PR incoming

[brandonduff]

From what we can tell, we never actually use filters in a create/update, except in VMP where we don't actually use BrainstemJS's fetching.

[juanca]

Yeah, I don't think filters matter now since create and update only return a single object as a result (besides anything for include).

Does the test already exist in the codebase or are we considering adding it?

[brandonduff]

We were gonna add it. We've sort of re-thought this.

You're right, in the sense that using a "filter" when creating/updating a single object doesn't make sense. However, our use-case involves sending a query parameter over with the update so that the response has the right information. Basically, the presentation of an object is relevant to a date, which product wants to always be today according to the timezone of the user's browser.

We are able to get around this by using the params option, which probably makes more sense. However, our adapter we use in VMP (which is intended to have the same interface as Brainstem.js) allows for filters to work, which confused us a bit.
mavenlink/frontend/brainstem-redux-adapter/make-adapter.js

Either way, I think we should either make handling filters do something useful (rather than translating it to "[object Object]") or just ignore the option if doing a create/update. What do you think?

[juanca]

our use-case involves sending a query parameter over with the update so that the response has the right information

Can you provide the example request and response? It sounds like perhaps the required parameter should be part of the model attributes?

However, if we think it is not relevant to put the parameter in the attributes, I think the params options might be fine -- it should also work with brainstem-js.

[brandonduff]

Yeah, we've used params and it works fine. The endpoint we're using is app/controllers/api/v1/views/budget_estimates_controller.rb.

I guess for this use case, it seems that for create/update either filters should not be a valid option, and just params should be used (which is what we're currently doing), or filters should actually work.

It also may make sense to make it part of the model attributes. It's used as a query param for the show action, and we're using it in the same way in the update, so we thought it made sense for a query param there as well.

EDIT: For some backstory, the thing we're dealing with here is what we're calling a BudgetEstimate for a workspace, which relies on the "Estimate to Complete" for that workspace and prevents data that's deduced from it. In most use-cases, Estimate to Complete will be calculated based off of either hard allocations or scheduled hours, but for the contracted customer, we're allowing them to manually set it (and thus we're doing that use-case first).

The thing about the BudgetEstimate is we need to know from what day we're estimating. Essentially, the same day could have actuals (time entries/expenses) and planned (SADs/Allocations), so we have to decide a cut-off point where we look at all line items effective before that date and all planned hours from that date and after. That is why we must supply an effective date for a BudgetEstimate.

That's probably more backstory than is needed for this issue, and it just happens that we ran across this behavior in brainstem-js and thought we should do something about it since it appears broken and confused us for a bit.

[juanca]

Okay, I think the way to tackle this issue is to throw when create / update has some filters. We should not allow it since it does not make sense to filter a created or updated model.