✨(api) add xAPI statement forwarding background task #199

SergioSim · 2022-06-20T10:43:37Z

Purpose

We want to support xAPI statement forwarding to make ralph able to
forward statements it receives to other Learning Record Stores.
For now, we only support forwarding statements using HTTP basic auth.

Proposal

Add xAPI statement forwarding

src/ralph/api/routers/statements.py

jmaupetit

Looks amazing!

This feature is not documented in the LRS specification?
Should we consider rule-based forwarding to filter which statement should be forwarded to which LRS?

src/ralph/api/forwarding.py

src/ralph/api/routers/statements.py

src/ralph/defaults.py

setup.cfg

quitterie-lcs · 2022-06-22T16:27:49Z

src/ralph/api/forwarding.py

+        min_anystr_length = 1
+
+    url: AnyUrl
+    is_active: Literal[True]


Why are you using a Literal ?

The idea is to consider forwarding configurations that have a value other than True as invalid and skip those in get_valid_xapi_forwarding.

Updated this part) as we now use pydantic settings management, deactivated forwarding configurations need to be considered valid, thus the is_active field became bool, and we manually skip deactivated forwardings in the get_active_xapi_forwardings method.

quitterie-lcs · 2022-06-22T17:03:13Z

tests/api/test_forwarding.py

+        ),
+    ],
+)
+def test_api_forwarding_get_valid_xapi_forwarding_with_valid_and_invalid_values(


I propose a fixture for this test but not sure if this would work. Create a fixture with default values and then you can overwrite the values (for this test is_active to False and also specify which field to ignore, with an input parameter ignored as a list of fields. Maybe it's too overthought ?

I like the idea) however, I don't find an easy way to achieve it as this test uses lists of variable length containing forwarding configurations. I now tried to split this test into more simple tests to benefit from the hypothesis fixtures. WDYT?

I think that is a good idea to split into more simple tests indeed. It would strengthen the test viability. If that is good for you, let's go!

SergioSim · 2022-06-23T11:42:04Z

This feature is not documented in the LRS specification?

Yes, it seems xAPI statement forwarding is not a standard (required) LRS feature.

Should we consider rule-based forwarding to filter which statement should be forwarded to which LRS?

Great idea) Would simple rules like (path_to_field, exact_value_match) be sufficient?

jmaupetit · 2022-06-23T14:54:28Z

Great idea) Would simple rules like (path_to_field, exact_value_match) be sufficient?

Afterthought: I don't think we'll have to implement this feature in a near future. If this is required, we can use https://github.com/yetanalytics/xapipe

quitterie-lcs · 2022-08-04T13:42:46Z

docs/api.md

+### Forwarding statements
+
+Ralph's API server can be configured to forward xAPI statements it receives to other
+Learning Record Stores.


On line 2 of this document, could you replace "LRS" with "Learning Record Store (LRS)" and use here the "LRS" acronym. Just for the documentation consistency 😉

Good Idea) Thanks) update this part with "LRSes")

quitterie-lcs · 2022-08-04T13:54:10Z

tests/api/test_forwarding.py

+def test_api_forwarding_forward_xapi_statements_with_unsuccessful_request(
+    monkeypatch, caplog, statements, config
+):
+    """Tests the `forward_xapi_statements` function should log the the error if the


Suggested change

"""Tests the `forward_xapi_statements` function should log the the error if the

"""Tests the `forward_xapi_statements` function should log the error if the

Well spotted) Thanks) updated this part)

quitterie-lcs · 2022-08-04T14:37:07Z

tests/api/test_statements_post.py

+                    url="http://localhost:8101/xAPI/statements/",
+                    is_active=True,
+                    basic_username="ralph",
+                    basic_password="admin",
+                    max_retries=1,
+                    timeout=10,


Could it be (an) input configuration parameter(s) ?

Good point) Indeed we could define this part using hypothesis; however, I'm hesitating as now the test has changed and already contains nine arguments.

The ES backend query_statements_by_ids method should only search for documents under the specified index and not in the whole ES database. We also test this behaviour for the Mongo database to avoid future surprises.

jmaupetit

That's brilliant! Tests should have been tricky to write 😍

docs/api.md

setup.cfg

docs/api.md

src/ralph/api/forwarding.py

src/ralph/conf.py

src/ralph/api/routers/statements.py

tests/api/test_forwarding.py

tests/fixtures/backends.py

jmaupetit

Merge this! Thx!

We want to support xAPI statement forwarding to make ralph able to forward statements it receives to other Learning Record Stores. For now, we only support forwarding statements using HTTP basic auth.

SergioSim force-pushed the add-statement-forwarding branch from 3c8d751 to fbeafa9 Compare June 20, 2022 13:03

SergioSim requested review from jmaupetit and quitterie-lcs June 20, 2022 13:10

SergioSim commented Jun 20, 2022

View reviewed changes

src/ralph/api/routers/statements.py Outdated Show resolved Hide resolved

jmaupetit assigned SergioSim Jun 21, 2022

jmaupetit added the feature label Jun 21, 2022

jmaupetit reviewed Jun 21, 2022

View reviewed changes

src/ralph/api/forwarding.py Outdated Show resolved Hide resolved

src/ralph/api/routers/statements.py Outdated Show resolved Hide resolved

src/ralph/defaults.py Outdated Show resolved Hide resolved

setup.cfg Show resolved Hide resolved

quitterie-lcs reviewed Jun 22, 2022

View reviewed changes

SergioSim force-pushed the add-statement-forwarding branch 3 times, most recently from cd4e496 to 65abc34 Compare June 28, 2022 17:24

quitterie-lcs reviewed Aug 4, 2022

View reviewed changes

🐛(backends) fix ES backend query_statements_by_ids method

4d07214

The ES backend query_statements_by_ids method should only search for documents under the specified index and not in the whole ES database. We also test this behaviour for the Mongo database to avoid future surprises.

SergioSim force-pushed the add-statement-forwarding branch 3 times, most recently from f2c75e1 to 15260fb Compare August 31, 2022 17:56

SergioSim added the needs review label Aug 31, 2022

SergioSim force-pushed the add-statement-forwarding branch from 15260fb to 71f9a1b Compare September 1, 2022 06:46

SergioSim requested review from jmaupetit and quitterie-lcs September 1, 2022 07:03

jmaupetit reviewed Sep 1, 2022

View reviewed changes

SergioSim force-pushed the add-statement-forwarding branch from 9bcb356 to b43c4c0 Compare September 1, 2022 12:09

jmaupetit approved these changes Sep 1, 2022

View reviewed changes

✨(api) add xAPI statement forwarding background task

92f53e7

We want to support xAPI statement forwarding to make ralph able to forward statements it receives to other Learning Record Stores. For now, we only support forwarding statements using HTTP basic auth.

SergioSim force-pushed the add-statement-forwarding branch from 2564871 to 92f53e7 Compare September 2, 2022 09:11

SergioSim merged commit 7645139 into openfun:master Sep 2, 2022

SergioSim deleted the add-statement-forwarding branch September 2, 2022 09:17

	"""Tests the `forward_xapi_statements` function should log the the error if the
	"""Tests the `forward_xapi_statements` function should log the error if the

✨(api) add xAPI statement forwarding background task #199

✨(api) add xAPI statement forwarding background task #199

Uh oh!

Conversation

SergioSim commented Jun 20, 2022

Purpose

Proposal

Uh oh!

Uh oh!

jmaupetit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergioSim commented Jun 23, 2022

Uh oh!

jmaupetit commented Jun 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmaupetit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmaupetit left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants