Settings: Metric Configuration: Save Definitions (and Replace Mocks) (6/n)

[mxosman]

Description of the change

Implements functionality to save includes/excludes definition responses and update UI with the changes.

MetricConfig-SaveDefinitions.mov

Related issues

Closes #82

Checklists

Development

This box MUST be checked by the submitter prior to merging:

• Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

• Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

• This pull request has a descriptive title and information useful to a reviewer
• This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
• Potential security implications or infrastructural changes have been considered, if relevant

[lilidworkin]

I am going to leave this one to @terryttsai to review since it's heavier on logic! I will say that I definitely think we can/should invest in a store + refactor after this. We should definitely have the time this week!

[lilidworkin]

Would it make sense to store settings mapped by key? e.g. instead of settings?: MetricConfigurationSettings[] it would be settings: Dict[str, MetricConfigurationSetting]. Not only would this be faster, I feel like it would make the code simpler?

[mxosman]

Personally, it feels easier and quicker to map through the array itself - the render is quicker too because it'll just map through the array itself. To me it feels like mapping an array by key and using it always requires an extra step in taking an existing array, mapping it to the object, and then eventually you'll have to converting that mapped object back to an array to render the list. Happy to continue discussing this! Maybe there's something I'm not seeing/understanding?

In L172 - it's taking the first item in the array because it's only updating one item at a time (with the exception of reverting to default) - so there will only ever be one item.

If we are able to create a store and reorganize, I think the UI update logic would naturally be simplified - but definitely a big refactor as we'll also need to update how things are rendered. If we have time to work on a store, I would love some help because it will be a bit of a beast. @terryttsai - if you have any interest/capacity - I would love to work together on this and figure out the best way to refactor and reorganize into a store.

[lilidworkin]

I think I see what you're saying about rendering being easier if we store as an array, but I'm still not sure I agree with the "quicker" part -- converting an array to a map is a one-time O(n) operation, and the benefit of paying that one-time cost is that it means all lookups can be O(1) instead of O(n). In line 171, we're iterating through all settings and looking for the one that matches, so that's O(n), whereas it could just be O(1). In practice, with our data size, that really doesn't matter -- I was more thinking that the code could be cleaner because you could just do something like this:

    settings = prev[metricKey].settings
    settings[updatingSetting.key] = updatingSetting
    return {
      ...prev,
      [updatedSetting.key]: {
        ...prev[metricKey],
        settings: settings,
      },

It's quite possible what I'm suggesting does not make sense or does not work in the JS world, though!

Also 100% to you and @terryttsai working together on a refactor, if you both agree that it makes sense!

[mxosman]

Oh, I should've used a better word - when I said "quicker" I definitely was not referring to time complexity at all! I should've said something like "fewer transformational (from array > object > array) steps in my mind".

Thank you for clarifying how you were seeing this - very helpful to see the example! This makes sense and absolutely could work in JS world. I can try that and let you know how it works - definitely agree that it is cleaner!

[mxosman]

Although, I don't think I can actually just try this within this current structure - but, will keep this in mind for when we can refactor to mapping objects such as this to a key: value structure and storing them to get the benefit of a quick lookup as you explained!

[mxosman]

    settings = prev[metricKey].settings
    settings[updatingSetting.key] = updatingSetting
    return {
      ...prev,
      [updatedSetting.key]: {
        ...prev[metricKey],
        settings: settings,
      },

One thing I noticed in this example - we would have to remap the settings object back to an array (i.e. Object.values(settings)) before we return the updated object - would that then add another O(n) operation or am I confusing myself now?

[lilidworkin]

I was imagining we'd change the type in the store (or wherever it gets sent back to) as well! But yea, this is just an idea to think about moving forward, it's certainly not blocking.

[terryttsai]

So although rendering from an array is a bit nicer than having to convert an object into an array first, if we're updating values in the array by key, like in this metric settings array, I would store the array as a dict instead, so our updaters look like what Lili suggested above, this makes it clearer what we're intending to update and it's more efficient than looping through the entire array each time we update.

When we render, at that point it can be converted back into an array for rendering.

[mxosman]

This all makes sense - thank you both for sharing your thoughts on a cleaner way to implement this with a dictionary!

[terryttsai]

What settings update causes this to be triggered?

[mxosman]

This kicked my butt on Friday - I couldn't figure out why certain things were not updating and it's because activeDimension needs to also update when there's a change to the metricSettings. The MetricDefinitions component currently relies on an up to date activeDimension.

[terryttsai]

So looking at the updateMetricSettings method, I'm not caught up on what we're thinking of for a refactor, but I can imagine these update methods being part of the metric settings mobx store and any component can call it and update the metric settings structure?

And furthermore that store can provide all of these metric configuration components the metric settings data + updaters, and there won't be a need to pass individual Xstate and setXstate react state objects and setters between all these components?

[mxosman]

Would love to sync on this - haven't formulated any plans yet! But yes exactly, the way I'm thinking about this is inline with what you described - a bit similar to the data entry store and how it updates.

[terryttsai]

So although rendering from an array is a bit nicer than having to convert an object into an array first, if we're updating values in the array by key, like in this metric settings array, I would store the array as a dict instead, so our updaters look like what Lili suggested above, this makes it clearer what we're intending to update and it's more efficient than looping through the entire array each time we update.

When we render, at that point it can be converted back into an array for rendering.

[terryttsai]

Code in its current paradigm looks good to me for playtesting!

[lilidworkin]

Thanks for reversing the order of these! I do actually think this is a lot better! The way I think about it is, before when when reading it, my brain goes "Ok lots of stuff is happening here if the key matches, but there's a nagging curiosity in my head about what happens when it doesn't match" vs now it's like "Ok, I know what happens when the key is NOT equal -- nothing happens -- now I can focus all my energy on what happens when the key DOES match!"

[mxosman]

The way you explain it makes so much sense!