Skip to content
New issue

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

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

Already on GitHub? # to your account

[SQL-261] Reactions tutorial rewrite #428

Merged
merged 22 commits into from
Nov 26, 2024
Merged

[SQL-261] Reactions tutorial rewrite #428

merged 22 commits into from
Nov 26, 2024

Conversation

kelvinqian00
Copy link
Collaborator

@kelvinqian00 kelvinqian00 commented Oct 4, 2024

The current Reactions tutorial mainly uses JSON examples, but JSON is mainly used as an internal storage format. This rewrite uses screenshots from the Reactions UI, which the user actually interfaces with, on the Reactions page. Meanwhile, a description of the internal reactions spec (based on the tables used by the xAPI statement and profile specs), along with the JSON example, have been moved to the developer documentation page.

@kelvinqian00 kelvinqian00 marked this pull request as ready for review October 23, 2024 15:21
doc/dev.md Outdated
@@ -117,4 +117,124 @@ java -cp bench.jar lrsql.bench [arguments]

Sample insert and query inputs can be found in the distribution at `bench/`

### Reaction JSON
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm honestly not sure if this the dev documentation page is best place to put the reactions spec, since it makes it pretty long. Instead would it be a better idea to make a separate page for the spec?

Copy link
Member

Choose a reason for hiding this comment

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

I like separate page better I think

Copy link
Member

Choose a reason for hiding this comment

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

Also, it is technically not just a private API, because we describe all the other "private" endpoints pretty well. So maybe the reactions spec goes in a subfolder of the main docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@kelvinqian00 kelvinqian00 Oct 23, 2024

Choose a reason for hiding this comment

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

These screenshots will be replaced with ones from the updated UI once that PR is merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kelvinqian00 kelvinqian00 changed the title Reactions tutorial rewrite [SQL-261] Reactions tutorial rewrite Oct 23, 2024
Copy link
Member

@milt milt left a comment

Choose a reason for hiding this comment

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

Approved, maybe pending breaking out the reaction spec.

Copy link
Member

@cliffcaseyyet cliffcaseyyet left a comment

Choose a reason for hiding this comment

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

Very nice.

@@ -8,166 +8,75 @@ Reactions allow SQL LRS to watch for patterns in submitted xAPI data and dynamic

Copy link
Member

Choose a reason for hiding this comment

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

Since I did a full review, there's a typo on line 5. It should be "Reactions allows"

Copy link
Collaborator Author

@kelvinqian00 kelvinqian00 Nov 12, 2024

Choose a reason for hiding this comment

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

Oh I keep thinking "Reactions" was plural. I always forget "Reactions" is also a copyrighted/trademarked noun referring to the whole thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doc/reactions.md Outdated

### Conditions

`conditions` is a mapping of names to rules for finding significant statements. Rules can be composed with boolean logic.
Each condition is a set of rules for finding significant statements. Each condition has a unique name followed by its rules, which can be composed with boolean logic.
Copy link
Member

Choose a reason for hiding this comment

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

'significant' => 'relevant'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doc/reactions.md Outdated

In the example given above statement `a` must have an object id equal to `https://example.com/activities/a`, a verb id equal to `https://example.com/verbs/completed`, and a result success equal to `true`. Statement `b` must have the same verb and result success but an object id equal to `https://example.com/activities/b` and a timestamp greater than that of `a`.
![reaction edit alpha](images/reactions/edit_condition_alpha.png)
In the example given, statement "alpha" must have an object `id` equal to `https://example.com/activities/alpha`, a verb `id` equal to `https://example.com/verbs/completed`, and its result `success` property equal to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

just to illustrate the logic, I would write this as:

must have an object 'id' equal .... AND a verb 'id' equal to... AND 'result.success' property equal to

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doc/reactions.md Outdated
* OR: Array of rules of which one must be true
* NOT: Rule that must _not_ be true

Rule types (either "Statement Criteria" or a boolean) can be selected using the topmost select input, or via the "Add sub-clause" button.
Copy link
Member

Choose a reason for hiding this comment

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

insert a section after boolean about nesting and how you can go as deep as you need to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kelvinqian00 kelvinqian00 merged commit 28d6e09 into main Nov 26, 2024
14 checks passed
@kelvinqian00 kelvinqian00 deleted the reactions-tutorial branch November 26, 2024 16:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants