fix: Normalising Children and New Nodes #111

aerial-ace1 · 2022-10-11T12:12:12Z

What

Code added normalises the nodes using the normalising function in the API calls of:

Created
Updated Nodes
Updated Nodes Children

Fixes bug(s)

Fixes Children and new node id should be normalized in the backend #79

aadarsh-ram

@aerial-ace1 Thank you for your contribution :) I think this PR requires a few changes which need to be addressed, after which it can be merged.

backend/editor/entries.py

aadarsh-ram

Just a minor comment, I think I'll add it.

backend/editor/entries.py

aadarsh-ram

LGTM, but I'd like @alexgarel to review this.

alexgarel

Hi @aerial-ace1 great job for you spotted more than the ticket was saying

Still I would like you to make some fix, and we will be good.

alexgarel · 2022-10-14T12:51:09Z

backend/editor/entries.py

@@ -18,12 +18,16 @@ def create_node(label, entry, main_language_code):
    """
    Helper function used for creating a node with given id and label
    """
+    #Normalising new Node ID
+
+    normalised_entry = normalizing(entry, main_language_code).split(':', 1)[1]


You should not make the split here, should you ?

alexgarel · 2022-10-14T12:55:32Z

backend/editor/entries.py

+
+                normalised_new_node_keys["tags_ids_"+keys_language_code] = normalised_value
+        else:
+            normalised_new_node_keys[keys] = new_node_keys[keys]


Suggested change

normalised_new_node_keys[keys] = new_node_keys[keys]

# no need to normalize

normalised_new_node_keys[keys] = new_node_keys[keys]

alexgarel · 2022-10-14T12:55:59Z

backend/editor/entries.py

@@ -173,13 +177,28 @@ def update_nodes(label, entry, new_node_keys):
            continue
        query.append(f"""\nREMOVE n.{key}\n""")

+    #Normalising Node to be Updated


Suggested change

#Normalising Node to be Updated

# Adding normalized tags ids corresponding to entry tags

alexgarel · 2022-10-14T12:56:53Z

backend/editor/entries.py

@@ -173,13 +177,28 @@ def update_nodes(label, entry, new_node_keys):
            continue
        query.append(f"""\nREMOVE n.{key}\n""")

+    #Normalising Node to be Updated
+    normalised_new_node_keys = {}
+    for keys in new_node_keys.keys():


you should name it key, not keys

alexgarel · 2022-10-14T13:00:05Z

backend/editor/entries.py

+        if keys.startswith("tags") and not keys.endswith("str"):
+            if "ids" not in keys:


add the "_" to make the condition even more specific and explicit

Suggested change

if keys.startswith("tags") and not keys.endswith("str"):

if "ids" not in keys:

if keys.startswith("tags_") and not keys.endswith("_str"):

if "_ids_" not in keys:

# generate tags_ids from values

makes sense

alexgarel · 2022-10-14T13:01:24Z

backend/editor/entries.py

+                    normalised_value.append(normalizing(values, keys_language_code))
+                normalised_new_node_keys[keys] = new_node_keys[keys]
+
+                normalised_new_node_keys["tags_ids_"+keys_language_code] = normalised_value


I would add a else to make explicit it's intended

Suggested change

normalised_new_node_keys["tags_ids_"+keys_language_code] = normalised_value

normalised_new_node_keys["tags_ids_"+keys_language_code] = normalised_value

else:

pass # we generate tags_ids, and ignore the one sent

alexgarel · 2022-10-14T13:03:19Z

backend/editor/entries.py

@@ -206,7 +225,13 @@ def update_node_children(entry, new_children_ids):
    existing_ids = [record['child.id'] for record in get_current_transaction().run(query, ids=list(added_children))]
    to_create = added_children - set(existing_ids)

-    for child in to_create:
+    #Normalising new children node ID


pep8 ask for space after the #

Suggested change

#Normalising new children node ID

# Normalising new children node ID

alexgarel · 2022-10-14T13:05:01Z

backend/editor/entries.py

+    normalised_added_children = set()
+    for child_node in to_create:
+        child_language_code, new_child_name = child_node.split(":",1)
+        normalised_added_children.add(normalizing(new_child_name,child_language_code))


You have to put back language code !

Suggested change

normalised_added_children.add(normalizing(new_child_name,child_language_code))

normalised_added_children.add(

f"{child_language_code}:{normalizing(new_child_name, child_language_code)}")

but see below because I propose you to remove this part.

alexgarel · 2022-10-14T13:08:00Z

backend/editor/entries.py

+    #Normalising new children node ID
+    normalised_added_children = set()
+    for child_node in to_create:
+        child_language_code, new_child_name = child_node.split(":",1)
+        normalised_added_children.add(normalizing(new_child_name,child_language_code))


As create_node is used below and it already normalize id I don't think we need this (and this avoid duplication of code).

Instead I propose to build normalised_added_children while creating nodes (see below)

alexgarel · 2022-10-14T13:09:50Z

backend/editor/entries.py

+    for child in normalised_added_children:
        main_language_code = child.split(":", 1)[0]
        create_node("ENTRY", child, main_language_code)


I propose to instead do

Suggested change

for child in normalised_added_children:

main_language_code = child.split(":", 1)[0]

create_node("ENTRY", child, main_language_code)

normalised_added_children = []

for child in to_create:

main_language_code = child.split(":", 1)[0]

created_node = create_node("ENTRY", child, main_language_code)

normalised_added_children.append(created_node.id)

By the way we can use a shorter name like created_ids instead of normalised_added_children

alexgarel

LGTM @aerial-ace1,
I will just add one comment before merging :-)

Thanks, it's a much appreciated contribution.

backend/editor/entries.py

alexgarel · 2022-10-19T17:21:58Z

@aerial-ace1: a small tip: don't make changes on your fork's main branch, as it will prevent you from cleanly update you fork from upstream project. Create a new branch for each PR.

aadarsh-ram · 2022-10-19T17:57:21Z

Thanks for the contribution @aerial-ace1!🎉

aerial-ace1 · 2022-10-19T19:53:40Z

Sure will do so from now on @alexgarel. Was fun doing this!

aerial-ace1 requested a review from a team as a code owner October 11, 2022 12:12

github-actions bot assigned aerial-ace1 Oct 11, 2022

aadarsh-ram changed the title ~~Normalising Children and New Nodes~~ fix: Normalising Children and New Nodes Oct 11, 2022

aadarsh-ram requested changes Oct 11, 2022

View reviewed changes

Added Normalisation in update,create and update_children

36fea9a

aadarsh-ram reviewed Oct 11, 2022

View reviewed changes

backend/editor/entries.py Outdated Show resolved Hide resolved

aadarsh-ram approved these changes Oct 11, 2022

View reviewed changes

aerial-ace1 and others added 4 commits October 11, 2022 23:39

Added normalization to update_nodes

3eea818

minor edit

79b7e32

major fix

c03f3ca

Update backend/editor/entries.py

d0beb2c

aadarsh-ram requested a review from alexgarel October 12, 2022 12:19

alexgarel requested changes Oct 14, 2022

View reviewed changes

Fixed neccessary changes

521ad50

alexgarel approved these changes Oct 19, 2022

View reviewed changes

backend/editor/entries.py Outdated Show resolved Hide resolved

refactor: add comment in node creation

a8204a2

alexgarel merged commit 118a60f into openfoodfacts:main Oct 19, 2022

openfoodfacts-bot mentioned this pull request Dec 1, 2022

chore(main): release 1.0.0 #136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Normalising Children and New Nodes #111

fix: Normalising Children and New Nodes #111

aerial-ace1 commented Oct 11, 2022 •

edited by aadarsh-ram

Loading

aadarsh-ram left a comment •

edited

Loading

aadarsh-ram left a comment •

edited

Loading

aadarsh-ram left a comment

alexgarel left a comment

alexgarel Oct 14, 2022

aerial-ace1 Oct 19, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

aerial-ace1 Oct 19, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel Oct 14, 2022

alexgarel left a comment •

edited

Loading

alexgarel commented Oct 19, 2022

aadarsh-ram commented Oct 19, 2022

aerial-ace1 commented Oct 19, 2022

	#Normalising Node to be Updated
	# Adding normalized tags ids corresponding to entry tags

		if keys.startswith("tags") and not keys.endswith("str"):
		if "ids" not in keys:

-        if keys.startswith("tags") and not keys.endswith("str"):
-            if "ids" not in keys:
+        if keys.startswith("tags_") and not keys.endswith("_str"):
+            if "_ids_" not in keys:
+                # generate tags_ids from values

	#Normalising new children node ID
	# Normalising new children node ID

fix: Normalising Children and New Nodes #111

fix: Normalising Children and New Nodes #111

Conversation

aerial-ace1 commented Oct 11, 2022 • edited by aadarsh-ram Loading

What

Fixes bug(s)

aadarsh-ram left a comment • edited Loading

Choose a reason for hiding this comment

aadarsh-ram left a comment • edited Loading

Choose a reason for hiding this comment

aadarsh-ram left a comment

Choose a reason for hiding this comment

alexgarel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgarel left a comment • edited Loading

Choose a reason for hiding this comment

alexgarel commented Oct 19, 2022

aadarsh-ram commented Oct 19, 2022

aerial-ace1 commented Oct 19, 2022

aerial-ace1 commented Oct 11, 2022 •

edited by aadarsh-ram

Loading

aadarsh-ram left a comment •

edited

Loading

aadarsh-ram left a comment •

edited

Loading

alexgarel left a comment •

edited

Loading