Fix for cards losing their values when in "dirty" state & adding back highlight - 7.5.x

[SDScandrettKint]

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

The main bug linked shows that some datatypes do not persist card data that is entered but not saved, a feature that almost all datatypes have. This bug is present for three datatypes: String, URL and Node Value.
This PR fixes all three (details on each fix below).

Issues Solved

Main issue: #10027
URL datatype issues: #10592, #8451

Checklist

• Unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Ticket Background

• Sponsored by: Knowledge Integration
• Found by: @SDScandrettKint
• Tested by: @SDScandrettKint
• Designed by: @SDScandrettKint

Further comments

String dtype:
This bug was caused by the defaultValue of the string datatype being set to the i18n dictionary of language and value, causing the data entered in the string card to be reset by the empty default value dictionary. The change in this PR checks whether the active language value is an empty string, and if so then sets the default value as an empty string, allowing the data to persist once again.

URL dtype:
The values this.url and this.url_label are used in the front end HTM for the datatype, but the variable changed when entering data is this.value().url & url_label. The two were not visually synced, as navigating back and forth from the card would reset the UI (this.url & this.url_label) data but not the 'this.value().X' data. This fix updates the visual variables with what has been entered.
This PR also includes a fix for #10592 by raising an error if the URL label has been inputted without the URL itself.
It also includes a fix for #8451 by adding the url_label to the object with the value of an empty string.
Also found during testing this PR was #10732.

Node Value dtype:
When a node value is selected and navigated from and to, the $(el).val() in select2-query is not retained unlike the other select widgets (concept, resource-instance etc). Similarly, the initSelection function within select2config id value was reset each time the node was navigated away from. Assigning the self.value() to the id allows data to be retained.

[jacobtylerwalls]

Hey @SDScandrettKint thanks for this. I noticed there wasn't optional chaining after accessing arches.activeLanguage, so I gave it a spin and indeed can break the widget in this workflow:

• save string node
• save text widget
• switch language while in graph designer
• view text widget config

text-widget-crash.mov

[SDScandrettKint]

Thanks for pointing this out @jacobtylerwalls ! I've added optional chaining to the defaultValue arches.activeLanguage value.

I was doing some debugging to figure out why the changed language default value wasn't working, and it left me a bit confused. In the screenshot below, logging out params?.config?.()?.defaultValue (1) shows the "de" default value in the object, the arches.activeLangauge is "de" but the params?.config?.()?.defaultValue[arches.activeLanguage] doesn't exist (3) which somewhat contradicts the first.

When the active langauge is "en", params?.config?.()?.defaultValue[arches.activeLanguage] works fine. Does this only work for the default language i.e "en"?

[jacobtylerwalls]

Interesting, so it sounds like it's mutating in between your breakpoints. I had a couple items I was wondering if you gave some thought to:

1. Your diff sets defaultValue = "", which I think will only create an entry for the active language. Is that why you're seeing a different result for English only? What happens if you set all languages to the empty string there?
2. There's code below this in init() that makes determinations about the default value for all languages. Is there a deeper source for this bug in that logic?

[SDScandrettKint]

I have taken another look at it and believe the latest commit is a much better, simpler and overall nicer looking fix for the issue. It uses the currentDefaultText variable that already exists and interacts with the default value, and checks if that is a value or an empty string. If currentDefaultText is empty then self.defaultValue() can be made empty too, as otherwise it is remaining as populated with the i18n empty dictionary, which causes the original issue of the entered data not being retained.

This improved fix also means there is no need to access the current language, as this is already done in currentDefaultText, allowing for the value data to be looked at directly.

I have tested switching between languages, as demonstrated in your video, and that error no longer occurs.

[SDScandrettKint]

One extra bug I've noticed whilst testing is that "Cancel edits" doesn't work correctly in the URL datatype for an unsaved tile edit. Unlike the other datatypes, clicking "Cancel edits" does not remove the edits and revert to the an empty text field. "Cancel edits" does work once the tile has been initially saved.

url_datatype_cancel_edits_not_working.mp4

I have created an issue for this bug (#10732), but I think it seems relevant to look for a fix as a part of this PR. Just thought I'd make a note of it here.

[jacobtylerwalls]

I have created an issue for this bug (#10732), but I think it seems relevant to look for a fix as a part of this PR. Just thought I'd make a note of it here.

This PR is already addressing several issues, so if the fix turns out to be cleanly segregated from your changes here, it would be better to have a new PR for it. (It may be that it doesn't qualify for a backport to 7.5 if it's not a recent regression or related to data loss.)

[SDScandrettKint]

I have created an issue for this bug (#10732), but I think it seems relevant to look for a fix as a part of this PR. Just thought I'd make a note of it here.

This PR is already addressing several issues, so if the fix turns out to be cleanly segregated from your changes here, it would be better to have a new PR for it. (It may be that it doesn't qualify for a backport to 7.5 if it's not a recent regression or related to data loss.)

After a little bit of investigating this issue, I believe that it belongs in a separate PR as it seems like it can be cleanly separated from the work in this PR.

[jacobtylerwalls]

Nice work with the tests; they're clear and they fail on the target branch.

One thing that can improve the developer experience when tests fail is when all of the assertions have a chance to fail. For example here, when I check them on the target branch, the test stops at the first failed assertion.

If you find yourself doing something repetitive, like calling url.validate(), you can batch them up and call them under with self.subTest() so that the runner will actually assert everything instead of failing fast and exiting.

Here's an example, where the left side input= is an arbitrary label that I want to appear in my test output:

arches/tests/utils/datatype_tests.py

Lines 33 to 42 in e4145e4

	for good in ["true", "false", "yes", "no", None]:
	with self.subTest(input=good):
	no_errors = boolean.validate(good)
	self.assertEqual(len(no_errors), 0)
	for bad in ["garbage", "True", "False", "None"]:
	with self.subTest(input=bad):
	errors = boolean.validate(bad)
	self.assertEqual(len(errors), 1)

If you play with that example and change the expected result, you'll get all 9 failures with the info about the specific way each one failed. Versus if you remove the subTest() statement, you only get 1 failure.

We can merge this work without adding subTest IMO, but I just wanted to mention it in case you find this useful when writing other unit tests.

[jacobtylerwalls]

After '' is cleaned to None, the dirty state isn't as I would expect. But I can imagine futuring this to a new issue if it's not straightforward.

no-dirty-state-after-space-entry.mov

[jacobtylerwalls]

Thanks, fixes look great!

[bferguso]

@SDScandrettKint, @jacobtylerwalls - I was looking at this as it was causing me issues (of my own making) and in look at this I was wondering if line 184 should be:
self.defaultValue(initialDefault);
rather than setting the value to an empty string. I think (?) that the value and defaultValues should be the i18n object unless I've got that wrong... If my assumption is correct, not sure if this might bite us somewhere down the road...

[jacobtylerwalls]

I was wondering along the same lines here, but I remember being less concerned about it when I saw other logic that watches the value and supplies the missing language keys (I think?)

This whole viewmodel is pretty involved, so I'm hoping we can kind of just make it to the Vue cutover without having to refactor it.