Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 16 commits into from
Apr 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2ae5400
Add back unsaved edit css
SDScandrettKint Feb 5, 2024
1de9e81
Read i18n default value
SDScandrettKint Feb 5, 2024
d625b0f
URL datatype fixes for 10027, 10592, 10593
SDScandrettKint Feb 7, 2024
4fd4364
Assign id to self value to retain nodevalue data
SDScandrettKint Feb 27, 2024
39f34b6
reword comments
SDScandrettKint Feb 27, 2024
741e91c
add comment for node value fix
SDScandrettKint Mar 1, 2024
c1b57ac
Add optional chaining for activeLanguage value
SDScandrettKint Mar 5, 2024
c98045b
Improved fix for text widget defaultValue issue
SDScandrettKint Mar 6, 2024
7684842
Add pre_tile_save to urldatatype and move fix
SDScandrettKint Mar 7, 2024
aa9e9d9
remove AttributeError exception from url pre_tile_save
SDScandrettKint Mar 8, 2024
17bfad7
Simplify url js
SDScandrettKint Mar 11, 2024
2aedaa7
Guard against None url tile data
SDScandrettKint Apr 3, 2024
2e569c0
Adding trailing semicolons
SDScandrettKint Apr 3, 2024
685a5c6
Add URLdatatype tests and simplify pre_tile_save
SDScandrettKint Apr 10, 2024
fd3c389
Test additions
SDScandrettKint Apr 11, 2024
60c1af4
Added clean & pre_structure_tile_data functions, and tests
SDScandrettKint Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion arches/app/datatypes/url.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ class URLDataType(BaseDataType):

def validate(self, value, row_number=None, source=None, node=None, nodeid=None, strict=False, **kwargs):
errors = []

if value is not None:
try:
if value.get("url") is not None:
if value.get("url"):
# check URL conforms to URL structure
url_test = self.URL_REGEX.match(value["url"])
if url_test is None:
Expand All @@ -82,6 +83,14 @@ def validate(self, value, row_number=None, source=None, node=None, nodeid=None,
title = _("Invalid HTTP/HTTPS URL")
error_message = self.create_error_message(value, source, row_number, message, title)
errors.append(error_message)

# raise error if label added without URL (#10592)
if value.get("url_label") and not value.get("url"):
message = _("URL label cannot be saved without a URL")
title = _("No URL added")
error_message = self.create_error_message(value, source, row_number, message, title)
errors.append(error_message)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

return errors

def transform_value_for_tile(self, value, **kwargs):
Expand Down Expand Up @@ -255,3 +264,19 @@ def default_es_mapping(self):
},
}
}

def pre_tile_save(self, tile, nodeid):
if (tile_val := tile.data[nodeid]) and "url_label" not in tile_val:
tile_val["url_label"] = ""

def clean(self, tile, nodeid):
if (data := tile.data[nodeid]):
try:
if not any([val.strip() for val in data.values()]):
tile.data[nodeid] = None
except:
pass # Let self.validate handle malformed data

def pre_structure_tile_data(self, tile, nodeid, **kwargs):
if (tile_val := tile.data[nodeid]) and "url_label" not in tile_val:
tile_val["url_label"] = ""
6 changes: 6 additions & 0 deletions arches/app/media/css/tree.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@
margin-right: -2px;
}
}

.unsaved-edit {
background: #ffdb70;
color: #fff;
border-width: 2px;
}

a.tree-display-tool {
margin: 0px;
Expand Down
6 changes: 6 additions & 0 deletions arches/app/media/js/viewmodels/node-value-select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,12 @@ define([
initSelection: function(element, callback) {
var id = $(element).val();
var tiles = self.tiles();

// fixes #10027 where inputted values will be reset after navigating away
if (self.value()) {
id = self.value();
}

if (id !== "") {
var setSelection = function(tiles, callback) {
var selection = _.find(tiles, function(tile) {
Expand Down
4 changes: 4 additions & 0 deletions arches/app/media/js/views/components/widgets/text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ define([

});

if (self.currentDefaultText() === "") {
self.defaultValue("");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

};

return ko.components.register('text-widget', {
Expand Down
10 changes: 10 additions & 0 deletions arches/app/media/js/views/components/widgets/urldatatype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@ define([

WidgetViewModel.apply(this, [params]);


// #10027 assign this.url & this.url_label with value versions for updating UI with edits
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
if (ko.isObservable(this.value) && this.value()) {
var valueUrl = this.value().url;
var valueUrlLabel = this.value().url_label;
this.url(valueUrl);
this.url_label(valueUrlLabel);
}


this.urlPreviewText = ko.pureComputed(function() {
if (this.url()) {
if (this.url_label && this.url_label()) {
Expand Down
86 changes: 86 additions & 0 deletions tests/utils/datatype_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,89 @@ def test_tile_clean(self):
string.clean(tile2, nodeid)

self.assertIsNotNone(tile2.data[nodeid])


class URLDataTypeTests(ArchesTestCase):
def test_validate(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

url = DataTypeFactory().get_instance("url")

# Valid tile
no_errors = url.validate({"url": "https://www.google.com/", "url_label": "Google"})
self.assertEqual(len(no_errors), 0)
# Invalid URL
some_errors_invalid_url = url.validate({"url": "google", "url_label": "Google"})
self.assertEqual(len(some_errors_invalid_url), 1)
# No URL added - cannot save label without URL
some_errors_no_url = url.validate({"url_label": "Google"})
self.assertEqual(len(some_errors_no_url), 1)
# No URL added - with url empty string in object
some_errors_no_url = url.validate({"url": "", "url_label": "Google"})
self.assertEqual(len(some_errors_no_url), 1)

def test_pre_tile_save(self):
url = DataTypeFactory().get_instance("url")

nodeid = "c0ed4b2a-c4cc-11ee-9626-00155de1df34"
resourceinstanceid = "40000000-0000-0000-0000-000000000000"

url_no_label = {
"resourceinstance_id": resourceinstanceid,
"parenttile_id": "",
"nodegroup_id": nodeid,
"tileid": "",
"data": {nodeid: {"url": "https://www.google.com/"}},
}
tile1 = Tile(url_no_label)
url.pre_tile_save(tile1, nodeid)
self.assertIsNotNone(tile1.data[nodeid])
self.assertTrue("url_label" in tile1.data[nodeid])
self.assertFalse(tile1.data[nodeid]["url_label"])

url_with_label = {
"resourceinstance_id": resourceinstanceid,
"parenttile_id": "",
"nodegroup_id": nodeid,
"tileid": "",
"data": {nodeid: {"url": "https://www.google.com/", "url_label": "Google"}},
}
tile2 = Tile(url_with_label)
url.pre_tile_save(tile2, nodeid)
self.assertIsNotNone(tile2.data[nodeid])
self.assertTrue("url_label" in tile2.data[nodeid])
self.assertTrue(tile2.data[nodeid]["url_label"])

def test_clean(self):
url = DataTypeFactory().get_instance("url")

nodeid = "c0ed4b2a-c4cc-11ee-9626-00155de1df34"
resourceinstanceid = "40000000-0000-0000-0000-000000000000"

empty_data = {
"resourceinstance_id": resourceinstanceid,
"parenttile_id": "",
"nodegroup_id": nodeid,
"tileid": "",
"data": {nodeid: {"url": "", "url_label": ""}},
}
tile1 = Tile(empty_data)
url.clean(tile1, nodeid)
self.assertIsNone(tile1.data[nodeid])

def test_pre_structure_tile_data(self):
url = DataTypeFactory().get_instance("url")

nodeid = "c0ed4b2a-c4cc-11ee-9626-00155de1df34"
resourceinstanceid = "40000000-0000-0000-0000-000000000000"

data_without_label = {
"resourceinstance_id": resourceinstanceid,
"parenttile_id": "",
"nodegroup_id": nodeid,
"tileid": "",
"data": {nodeid: {"url": ""}},
}
tile1 = Tile(data_without_label)
url.pre_structure_tile_data(tile1, nodeid)
self.assertIsNotNone(tile1.data[nodeid])
self.assertTrue("url_label" in tile1.data[nodeid])
self.assertFalse(tile1.data[nodeid]["url_label"])
Loading