[D&D] Adds drop validation

[ashwin-pc]

Description

This PR adds validation to check if the field being dragged can be dropped in a dropbox and if so which default aggregation should be used.

Issues Resolved

Check List

• New functionality includes testing.
  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
• New functionality has been documented.
• Commits are signed per the DCO using --signoff

[joshuarrrr]

Some questions and comments, but feel free to merge and keep the ball rolling.

[joshuarrrr]

From a UX perspective, it seems like it would be better to determine the valid drop targets on the drag start event rather than waiting until the drop to declare it invalid. If we did that, we could highlight only valid drop targets, making it clear to the user where that field can go.

[ashwin-pc]

Agreed. The user will never see this error. It will fail silently and only be logged in the console. The isValidDropTarget property returned by this hook is what actually indicates to the user that the field is droppable. And that is set when the drag begins. I just threw an error here for the console instead of failing silently if the user decided to ignore the UI indicators, but ill revert that. The error does not make sense here anymore.

[joshuarrrr]

Got it, that's helpful context.

[joshuarrrr]

I'm still catching up to some of the concepts in d&d, but why would we want to create an aggConfig even if there are no allowedAggTypes in the schema? Is it because schemas aren't required to define allowedAggTypes? If that's the case, why couldn't we just set L133 to validAggTypes and avoid the fallback here?

[ashwin-pc]

Great question. To understand the logic here, let me back up a bit since it took me a while to figure it out myself. On app start, the agg service registers a variety of aggregation types like avg, max, etc. But each schema has filters for the agg types they support and each aggtype has filters for the field types they support. So validAggTypes is an array of all the agg types that the field being dragged can be used with, and if there is atleast one of them, then we should be able to drop the field into that dropbox.

allowedAggTypes is a subset of validAggTypes where the aggTypes defined in schema.defaults are also present in validAggTypes. So the logic is that we use the default aggType thats also present in validAggTypes, but if that isnt possible, we just use the first agg type from validAggTypes as a fallback.

[joshuarrrr]

nit - this name doesn't need to include Droppable - this test is more generic, something like isFieldAllowed or something like that. I also kind of think this would be more readable if written with reduce() instead of forEach().

[ashwin-pc]

Not a fan of reduce, but i do agree with the readability. Ive refactored this to just use the filter and map functions.

[joshuarrrr]

This hook should probably be a separate file, partly because it looks like it will require substantial unit tests to verify all the edge cases.

[ashwin-pc]

Good call. Ill leave it in here for this PR. I am yet to add another validation for min and max values. Once i do that, ill move it out with some tests.