Expand test coverage in FieldUninstallValidatorTest

Ive added a test for validating the previous revisions. The test creates 3 revisions for the node. The link prop is used in the second revision. So, the exception message should contain '2' as the revision id.
The test works fine in local. But for some reason, revision id is coming as '1' in the exception message in gitlab ci and the test is failing...

Unassigning for now

Setting to needs work since there is existing Merge Request.

@akhil babu Thanks for getting this started!

Picking this up and assigning to myself

@deepakkm very close! Just a couple small things. I will fix those so we can get this MR merged quickly. Thanks!

@deepakkm this looks good for the "Field is used in previous revision." case. Thanks!

I will merge if the tests pass

Only one of the 4 in the @todos in the codebase and in the issue summary is done? 😅

@tedbow Thanks for reviewing , @wim yes only 2 of them is done in this but as per discussion between me and ted this will go in pieces.
Once this MR is merged I’m going to open another MR for 1 of those tests. Hope that makes sense.

And yes this shouldn’t be RTBC’ed 😜

@wim leers yep, sorry, I chatted with @deepakkm about this but forgot to update the summary.

I think we should commit each test case in their own MR to be practical. If we get just 1 done that is a win.

So will merge each as I review. will be clear next time which case is covered when RTBC and merge.

I just merged Field is used in previous revision.

I updated the descriptions of the other test cases

@deepakkm I suggest you take on Field used in multiple entities of different types next

I think we should commit each test case in their own MR to be practical. If we get just 1 done that is a win.

💯👏

Great call! 😄

deepakkm → changed the visibility of the branch 3471028-Expand-test-coverage-module to hidden.

@deepakkm one more follow-up needs to be made, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

I also made another one you can work on also https://www.drupal.org/project/experience_builder/issues/3476891 🐛 FieldTypeUninstallValidator lists the same content revision 2x if it the default revision Active

I think this looks good. But want to review it with fresh eyes tomorrow

deepakkm → closed merge request !332

This looks good to me. Will merge if tests pass

Just changing the tag. It really isn't hyphenated.

@tedbow - are we good to close this or do we need to work on the 4th issue i.e Test with field that does not use dedicated storage?
i know as per our discussion this was a very rare scenario and not as urgent as others.

@deepakkm can you make the follow-up issue I asked for here https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

After that re #28 let's mark this issue postponed on 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active because that is might change field storage, and probably make sense to postpone writing the remaining test case till after that is decided.

Created the follow-up 🐛 FieldTypeUninstallValidator's error message needs to include the entity type and bundle of the default value field Active

Postoning on 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active since that might change on things are stored. This issue is still minor Priority. I think it got less important since we have covered 2 of the more likely cases in the tests that were already merged

The remainder is handled over at #3457504-56: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation → ! So … that means we can mark this fixed, because an MR did land here, a long time ago, in #25.