fix: JSONSchema generated forms for contexts #257

ShubhranshuSanjeev · 2024-09-30T15:07:23Z

Problem

Context forms were generated using lot of broken, clumsy pattern matching logic, parsing of values and generating final context json's logic was spread across utility functions.

Solution

Replaced pattern matching over string to guess the type of value with algebraic representation of JsonLogic.
Single Input component, which works seemlessly with the schema type of the dimensions and render appropriate fields along with handling parsing and input validation.

sauraww · 2024-10-22T13:21:34Z

crates/frontend/src/logic.rs

+impl Operand {
+    pub fn from_operand_json(value: Value) -> Self {
+        match value {
+            Value::Object(ref o) if o.contains_key("var") => Operand::Dimension(value),


If ref required here ?
Can we remove it ?

I want a reference.

sauraww · 2024-11-18T11:02:57Z

crates/frontend/src/logic.rs

+        Operands(
+            iter.into_iter()
+                .map(Operand::from_operand_json)
+                .collect::<Vec<Operand>>(),


Can we make a type for Vec as Operands as we made for Conditions ?

Didn't really understand the point.

sauraww · 2024-11-18T11:05:53Z

crates/frontend/src/pages/context_override.rs

+    let on_context_edit = Callback::new(move |data: (Context, Map<String, Value>)| {
+        let (context, overrides) = data;
+        let conditions =
+            Conditions::from_context_json(&context.condition.into()).unwrap();


Can we avoid using plain unwrap here ?

sauraww · 2024-11-18T11:06:14Z

crates/frontend/src/pages/context_override.rs

+    let on_context_clone = Callback::new(move |data: (Context, Map<String, Value>)| {
+        let (context, overrides) = data;
+        let conditions =
+            Conditions::from_context_json(&context.condition.into()).unwrap();


sauraww · 2024-11-18T11:06:59Z

crates/frontend/src/pages/context_override.rs


-            open_drawer("context_and_override_drawer");
-        });
+        open_drawer("context_and_override_drawer");


Can we create ENUMs for these strings , it's a little dangerous to keep it as strings

Drawers are going away in my next PR, so this will be redundant work.

ShreyBana · 2024-12-12T16:42:44Z

crates/frontend/src/components/condition_pills.rs

@@ -106,18 +86,18 @@ pub fn condition_expression(
                    </span>

                    {match operator {
-                        ConditionOperator::Between => {
-                            if filtered_vals.len() == 2 {
+                        Operator::Between => {


Looking at this, it begs the question, should we instead parse at the expression level?
You know something like: Expr::Between(o1, o2). That way, you could tie the operands & operator together.
Did you by any chance already give this a thought?

ShreyBana · 2024-12-12T16:57:18Z

crates/frontend/src/components/input.rs

+) -> Result<Value, String> {
+    match op {
+        Operator::In => match type_ {
+            JsonSchemaType::String => serde_json::from_str::<Vec<String>>(s)


Seems a bit repetitive, can we refactor this a little?

ShreyBana · 2024-12-12T17:14:33Z

crates/frontend/src/logic.rs

+    ) -> Result<Self, Self::Error> {
+        match operator {
+            Operator::Is => Ok(Operands(vec![
+                Operand::Dimension(json!({ "var": d_name })),


Hmm, a little odd that dimension is also an operand. 🤔

ShreyBana · 2024-12-12T17:27:02Z

crates/frontend/src/pages/context_override.rs

+                set_form_mode.set(Some(FormMode::Edit));
+                open_drawer("context_and_override_drawer");
+            }
+            Err(e) => {


At this point in the program, if this call errs out, there's no useful error we can report to the user to un-block them. So maybe, it makes sense to use some kind of front-end panic which let's us write code as if there was no chance of an error to begin with. What do you think?

ShreyBana · 2024-12-17T17:23:38Z

crates/frontend/src/logic.rs

+
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
+pub enum Expression {
+    Is(Variable, Constant),


Hmm, don't know if the variable should be part of the Expression, or the Condition 🤔. Somehow I feel like Condition wrapping a variable and an Expression(w/ just the consants) makes more sense.

ShubhranshuSanjeev force-pushed the fix/context-form branch 2 times, most recently from 25de1f1 to d86f41d Compare October 8, 2024 08:11

sauraww requested changes Oct 22, 2024

View reviewed changes

ShubhranshuSanjeev force-pushed the fix/context-form branch 2 times, most recently from 0cca50a to 5346aa1 Compare October 23, 2024 10:22

ShubhranshuSanjeev changed the title ~~Fix/context form~~ fix: JSONSchema generated forms for contexts Oct 23, 2024

ShubhranshuSanjeev marked this pull request as ready for review October 28, 2024 08:25

ShubhranshuSanjeev requested a review from a team as a code owner October 28, 2024 08:25

ShubhranshuSanjeev force-pushed the fix/context-form branch from 641186c to 90f420a Compare November 8, 2024 10:57

ShubhranshuSanjeev force-pushed the fix/context-form branch 3 times, most recently from 533787b to f6a3529 Compare November 18, 2024 09:47

sauraww requested changes Nov 18, 2024

View reviewed changes

ShubhranshuSanjeev force-pushed the fix/context-form branch from e8799f2 to 35209d9 Compare December 3, 2024 09:01

ShubhranshuSanjeev changed the base branch from main to feat/json-logic-repr December 4, 2024 07:46

ShubhranshuSanjeev force-pushed the feat/json-logic-repr branch from 71868e6 to cb1579a Compare December 4, 2024 07:46

ShubhranshuSanjeev force-pushed the fix/context-form branch 2 times, most recently from 15e6abd to c5fddd0 Compare December 4, 2024 07:50

ShubhranshuSanjeev force-pushed the feat/json-logic-repr branch from cb1579a to 26e8054 Compare December 4, 2024 07:55

ShubhranshuSanjeev force-pushed the fix/context-form branch from c5fddd0 to 82daef4 Compare December 4, 2024 09:19

ShubhranshuSanjeev force-pushed the feat/json-logic-repr branch from 26e8054 to a701528 Compare December 4, 2024 11:27

ShubhranshuSanjeev force-pushed the fix/context-form branch from 82daef4 to 6d6f2ef Compare December 4, 2024 11:28

ShubhranshuSanjeev force-pushed the feat/json-logic-repr branch 2 times, most recently from 9886f4d to 96705d5 Compare December 4, 2024 11:39

ShubhranshuSanjeev force-pushed the fix/context-form branch 2 times, most recently from 5db64f5 to 23db368 Compare December 4, 2024 11:57

Datron requested a review from sauraww December 9, 2024 07:29

ShubhranshuSanjeev changed the base branch from feat/json-logic-repr to main December 10, 2024 11:21

ShreyBana reviewed Dec 12, 2024

View reviewed changes

ShubhranshuSanjeev force-pushed the fix/context-form branch from 23db368 to 8c6d3c7 Compare December 17, 2024 08:42

ShubhranshuSanjeev force-pushed the fix/context-form branch from 8c6d3c7 to fbb4902 Compare December 17, 2024 14:00

ShubhranshuSanjeev requested a review from ShreyBana December 17, 2024 14:15

ShubhranshuSanjeev force-pushed the fix/context-form branch 2 times, most recently from d02a6e8 to 60eaeba Compare December 17, 2024 14:30

ShreyBana approved these changes Dec 17, 2024

View reviewed changes

ShubhranshuSanjeev force-pushed the fix/context-form branch from 485acde to 2908496 Compare January 6, 2025 13:15

feat: JsonLogic algebraic representation

313dd0f

ShubhranshuSanjeev force-pushed the fix/context-form branch from 2908496 to da2a36e Compare January 6, 2025 13:22

fix: JSONSchema generated forms for contexts

b3fd79f

ShubhranshuSanjeev force-pushed the fix/context-form branch from da2a36e to b3fd79f Compare January 6, 2025 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: JSONSchema generated forms for contexts #257

fix: JSONSchema generated forms for contexts #257

ShubhranshuSanjeev commented Sep 30, 2024 •

edited

Loading

sauraww Oct 22, 2024

ShubhranshuSanjeev Oct 23, 2024

sauraww Nov 18, 2024

ShubhranshuSanjeev Nov 18, 2024

sauraww Nov 18, 2024

sauraww Nov 18, 2024

sauraww Nov 18, 2024

ShubhranshuSanjeev Nov 18, 2024

ShreyBana Dec 12, 2024

ShreyBana Dec 12, 2024

ShreyBana Dec 12, 2024

ShreyBana Dec 12, 2024

ShreyBana Dec 17, 2024

fix: JSONSchema generated forms for contexts #257

Are you sure you want to change the base?

fix: JSONSchema generated forms for contexts #257

Conversation

ShubhranshuSanjeev commented Sep 30, 2024 • edited Loading

Problem

Solution

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

ShubhranshuSanjeev commented Sep 30, 2024 •

edited

Loading