Allow avoiding error returning when updating an llms post meta with the same value as stored in the db

[eri-trabiccolo]

Description

Fixes #909 and needed to fix gocodebox/lifterlms-rest#222
This PR, aside from some code reorganization, just adds this piece of code:
https://github.com/gocodebox/lifterlms/pull/2129/files#diff-3f0337a1db5180a72d1b5434e90b08b7b0f4b7ee5997ac4dd50efa7b6d3c21a8R1482-R1497

How has this been tested?

existing unit tests (doesn't introduce any drawback since the old behavior is the default one).
working on new unit tests.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

• My code has been tested.
• My code passes all existing automated tests.
• My code follows the LifterLMS Coding & Documentation Standards.

[eri-trabiccolo]

@gocodebox/engineering
while I work on unit tests, if you got time, may I have your opinion on the strategy?
Thanks

[thomasplevy]

This looks pretty good to me. This is such a messy area of the codebase that this honestly seems to make it more messy to me but refactoring the code into separate methods is a good step towards making all of this a bit more testable and less fragile to maintain longterm.

I haven't physically run any tests but it looks solid given that existing unit tests are passing and the new tests you've added work. However, I'd like to see a bit more test coverage added here, specifically:

1. Usage of the new param in set() method isn't covered at all.
2. empty_data error condition in set_bulk() isn't covered
3. invalid_data error condition in set_bulk() isn't covered
4. method is_meta_value_same_as_stored_value() is only tested for scalar values (strings). Tests could be written to test this method directly, ideally with every scalar type and at least one non scalar type being passed in. Using LLMS_Unit_Test_Util::call_private_method() so you can just pass stuff in directly.
5. You technically did not add anything related to setting post properties but there's no coverage on post properties here (I know other model tests pick it up but it wouldn't be too hard to add some post properties here while you're at it.

[eri-trabiccolo]

This looks pretty good to me. This is such a messy area of the codebase that this honestly seems to make it more messy to me but refactoring the code into separate methods is a good step towards making all of this a bit more testable and less fragile to maintain longterm.

Yeah

I haven't physically run any tests but it looks solid given that existing unit tests are passing and the new tests you've added work. However, I'd like to see a bit more test coverage added here, specifically:

1. Usage of the new param in set() method isn't covered at all.

True.

2. empty_data error condition in set_bulk() isn't covered
3. invalid_data error condition in set_bulk() isn't covered

Yeah, I haven't added unit tests for old code.
Tests added in 58b0165

4. method is_meta_value_same_as_stored_value() is only tested for scalar values (strings). Tests could be written to test this method directly, ideally with every scalar type and at least one non scalar type being passed in. Using LLMS_Unit_Test_Util::call_private_method() so you can just pass stuff in directly.

You're right.
Tested it un-directly with non scalar types in 184a13c
and improved in 3abbdd9

5. You technically did not add anything related to setting post properties but there's no coverage on post properties here (I know other model tests pick it up but it wouldn't be too hard to add some post properties here while you're at it.

Because this PR didn't change the behavior on post properties: old code.

I can add more tests coverage though <3

[thomasplevy]

@eri-trabiccolo I realize that you didn't add tests for old code but since you're moving that old code and writing code all around it it's a good opportunity to add some tests to that old code.

[eri-trabiccolo]

Yeah sorry, what I meant was that despite what it looks like the PR introduces very little changes, and those are tested.
But yeah improving the code coverage is always good you're right.

[eri-trabiccolo]

@thomasplevy added various tests.