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.

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

MACF-7: Add generic action so that custom flows can be seen and edited #142

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

pcandia
Copy link
Contributor

@pcandia pcandia commented Oct 21, 2020

No description provided.

…ited by user (#137)

* MACF-7: Initial commit to have a generic action that custom flow can be seen and edited

* Display json_editor action for unsupported callflows actions

* User regex to remove everything inside branckets when setting the module name
@pcandia pcandia self-assigned this Oct 21, 2020
Copy link
Contributor

@joristirado joristirado left a comment

Choose a reason for hiding this comment

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

Seems like our callflow modules list isn't filtered correctly as it picks up callflows as a callflow module:
new-action-error

We didn't think of schema references!
audio_macro-module-error
This won't be a quick fix but I think we need to support it.

@pcandia
Copy link
Contributor Author

pcandia commented Oct 21, 2020

@azefiel Ok, I fixed the first issue and I'm working on a fix for the second one.

pcandia and others added 3 commits January 19, 2021 17:44
-adding validateSubSchema property to to check if schemas has ref on the specified path,
if so return a list of refs otherwise return false
-adding getSubSchema property to return a promise that resolves after all the refs have been
fetched and return an object with the {shcema_key: schema} structure
-making success function async on  miscSetSchema in order to wait for the sub schemas while they are
being fetched if the validateSubSchema function returned an array of ref values otherwise assign
an empty object to the Refs var that is passed as the second jsoneditor.setSchema parameter
Copy link
Contributor

@joristirado joristirado left a comment

Choose a reason for hiding this comment

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

Looks like the last commit to misc.js changed the tab spacing, updating every single line in that file, and making it hard to read the diff.

Please revert @Ramos95 , and make to sure to apply the tab spacing specified here, before reapplying your changes.

@Ramos95
Copy link
Contributor

Ramos95 commented Feb 23, 2022

Yeah! sorry about that @azefiel, did not noticed the changes done by Prettier. I'll revert back and apply again the changes following the specifications.

This reverts commit 3b7cb37.

Revert the spacing issues
    -fixing spacing issues
    -using monster.series on miscGetSubSchema function
    to fetch all schema references and set them to the json editor
    -passing data schema and jsoneditor tom miscGetSubSchema function
    to set them on the monster.series callback when after all subschemas
    had been fetched
    -if schema has references call miscGetSubSchema otherwise set the
    schema direclty to jsoneditor
@pcandia pcandia assigned Ramos95 and unassigned pcandia Mar 15, 2022
Copy link
Contributor

@joristirado joristirado left a comment

Choose a reason for hiding this comment

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

It looks like you have not been properly introduced to MUI's coding standards. In this particular case, I'm talking about running a linter to adhere to a common coding format. Currently, if I run the linter on one of the updated files, here is what I end up with:
Capture
More info on how to setup the linter for your environment here, as well as if you want to run it directly in command line.

@Ramos95
Copy link
Contributor

Ramos95 commented Mar 16, 2022

sorry my bad, I will review and fix the linting errors, using the the eslintcr guidelines

Copy link
Contributor

@joristirado joristirado left a comment

Choose a reason for hiding this comment

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

I did a high level pass on the code as well as usability and this error popped up when reloading the editor:
jsoneditor-error
Seems like some references are missing on reload?

app.js Show resolved Hide resolved
i18n/en-US.json Outdated Show resolved Hide resolved
i18n/en-US.json Outdated Show resolved Hide resolved
i18n/en-US.json Outdated Show resolved Hide resolved
submodules/misc/misc.js Outdated Show resolved Hide resolved
submodules/misc/misc.js Outdated Show resolved Hide resolved
submodules/misc/misc.js Outdated Show resolved Hide resolved
submodules/misc/misc.js Outdated Show resolved Hide resolved
submodules/misc/misc.js Outdated Show resolved Hide resolved
submodules/misc/misc.js Outdated Show resolved Hide resolved
-bug found: JSON editor does not reopen after saving JSON with subschemas
-cause:sub  schemas are not saved on local sotorages as the main schema
-solution: save the sub schemas on local sotorage so they can be avaliable
when JSON editor is opened again
Copy link
Contributor

@joristirado joristirado left a comment

Choose a reason for hiding this comment

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

Doesn't look like schema validation works, here's an example:

Screen.Recording.2022-09-21.at.5.38.43.PM.mov

submodules/jsoneditor/jsoneditor.js Outdated Show resolved Hide resolved
Comment on lines 261 to 268
jsonEditorSaveSchemaLocally: function(data, selectedSchema, isSubSchema) {
var self = this;
if (isSubSchema) {
self.appFlags.callflowsListSubSchema[selectedSchema] = data;
} else {
self.appFlags.callflowsListSchema[selectedSchema] = data;
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering why is there a distinction between where sub schema's are stored?

Schema identifiers should be unique, unless I'm missing something here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I sotred them now on same object with its id as the property name

Comment on lines +300 to +310
//api call to get subschema
return function(callback) {
self.jsonEditorGetSchema({
data: {
schemaId: ref
},
success: function(data) {
callback(null, data);
}
});
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Since nested schemas could reference ones that were already pulled and saved locally, we could check our cache here first, to avoid performing unnecessary API requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that schemas are not stored separated it can check if it is already stored and avpid calling api again

-removing callflowsSubSchemaList from appFlags
-removing jsonEditorSaveSchemaLocally function
-now saving schema and subschemas on callflowsListSchema as one object
-using callflowsListSchema as store for all fetched schemas
-validate if schema is aready store to set it with its subschemas
-validate if schema's sub schemas are already store and only fetch the ones
that are still not stored on the chache
@Ramos95
Copy link
Contributor

Ramos95 commented Oct 14, 2022

About the shema validation, the editor did not validate that the property beacuase it was not present since in the schema it is not defined as required, but if the 'macros' property is defined it validates that it should contain at least one item. So I'm not really sure how to handle the validation 🤔 like this case

Captura desde 2022-09-21 21-12-57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants