fix: add or update fields in the existing environment or rulesets

[luvsaxena1]

This is to fix issue issue. This could potentially fix adding nested object in the rulesets as well.

[klutchell]

LGTM, can we get this merged?

[luvsaxena1]

@klutchell I have tested this issue on my local version of safe settings and its working as expected.

It not only resolved issue 660 but also solved issue 655

[klutchell]

WDYT @decyjphr ?

[Brad-Abrams]

Reviewing

[Brad-Abrams]

This is tricky

@luvsaxena1 did you happen to see PR #649 Environments tolerate concise configuration? It combines together other PRs you have commented in, and it also addresses the issue you recently raised in #660. I confirmed this by locally adding a new unit test for the scenario you outlined in 660.

When I made the changes for #649, I was intentionally constraining the area of impact to environments in the hope that this would make the PR easier to review and accept.

That being said, I also like that you have gone after the broader change in mergeDeep

I've brought just the changes from this PR #665 locally, and run the updated unit tests for environments from PR #649. There is certainly an improvement that has come to environments from the mergeDeep changes. However there are still 5/12 unit tests which fail.

I'll give this some further thought and update back here

[decyjphr]

I know the mergeDeep is not optimal and could be improved. So adding this extract bit to do handle nested objects when adding elements to additions is great. Thanks.

[decyjphr]

@luvsaxena1 do you want us to take a look at resolving the tests or you are looking at it already? Don't want to muddy the waters...

[decyjphr]

LGTM

[decyjphr]

Tested locally and the tests seem to pass.

[Brad-Abrams]

@luvsaxena1 do you want us to take a look at resolving the tests or you are looking at it already? Don't want to muddy the waters...

@decyjphr, I think that perhaps this question is for me to answer. If #649 is also approved and merged, then I anticipate that it will take care of resolving the tests. I'll confirm and follow up here.

[Brad-Abrams]

@luvsaxena1 do you want us to take a look at resolving the tests or you are looking at it already? Don't want to muddy the waters...

@decyjphr, I think that perhaps this question is for me to answer. If #649 is also approved and merged, then I anticipate that it will take care of resolving the tests. I'll confirm and follow up here.

@decyjphr confirming that this PR and #649 are compatible. Combining the two still results in the expanded set of environments unit tests all passing.

[klutchell]

Are we set to merge this next? Now that #649 has merged it would be nice to get the fixes to mergeDeep to catch changes to rulesets and nested objects in general.

[luvsaxena1]

@decyjphr Can we merge this PR next ? As I said this also solve problem with nested objects of rulesets and other plugins.

[klutchell]

@decyjphr would it be too much trouble to drop an rc release with this change? ❤️

[decyjphr]

@klutchell I created 2.1.11-rc.2 hope that helps

[klutchell]

Thanks!