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

Re-enabled save button for task status node #1444

Merged
merged 4 commits into from
Jun 22, 2020

Conversation

diegocortassa
Copy link
Contributor

This button was removed 15 months ago by commit a2aeee1 not sure why, but it works and task status nodes can't be saved without it, perhaps at the time some code was not working. If there are any other reasons for the removal please excuse this PR.

@remkonoteboom
Copy link
Contributor

Yes, there is a good reason it was removed. All of the save buttons on the right "info" panels are no longer needed as the entire state of the workflow is stored on the nodes themselves as they are edited. This means that all you have to do is save the "pipeline" and everything will be saved with it.

Actually, the save button will mess up the state of the workflow as it will save to the database without updating the in memory state, which may then be erased the next time you save the pipeline (depending on various things).

If you have custom nodes, then at the top, you can add:

    top.add_class("spt_section_top")
    SessionalProcess.add_relay_session_behavior(top)

This will automatically look for form elements and save their state on the node.

@diegocortassa
Copy link
Contributor Author

I see, thanks for your answer it is much more clear now.
I don't have custom nodes but the task status node is not saved.
I changed the PR to use add_relay_session_behavior for TaskStatusInfoWdg.

There is still one thing I don't like in this PR: add_relay_session_behavior saves the workflow data in the section_name='default' json key while in workflow.py looks for them in the json root, to make it work had to copy "mapping" "direction" and "status" keys from "default" to json root.

Perhaps the ison path in workflow.py should be changed but I'm not sure if they are accessed in other places!

Please review my code in lines 7155-7179

@remkonoteboom
Copy link
Contributor

I think this was an oversight. The change in data structure might have some repercussions, specifically code dealing with those values. For the other nodes, we made the update handle it without anyone noticing.

@remkonoteboom
Copy link
Contributor

Sorry that I have left this Pull request open. I need to take some time to have a deeper look at the repercussions of changing the data structure.

@diegocortassa
Copy link
Contributor Author

No problem! Still testing myself, I just needed some review from you. It seems I can simplify this PR, changes to NewProcessInfoCmd() can be reverted I will commit what I think is a better solution soon.

@listyque
Copy link
Contributor

Diego. How do You handle Tasks Pipeline saving? Any progress on that?

@remkonoteboom
Copy link
Contributor

I have tested this myself and it seems to work. The only issue I have found is that it produces a process json that looks like this:

{
  "default": {
    "color": "#fa2929",
    "mapping": "Complete",
    "node_type": "manual"
  },
  "mapping": "Complete",
  "node_type": "status",
  "version": 2
}

Node type should not be "manual" and this should be fixed. I will add a new issue on this because I think that as it stands, this pull requests fixes an important issue and the the issue above, while not correct does not affect the behavior of the application.

@remkonoteboom remkonoteboom merged commit e673f8d into Southpaw-TACTIC:4.8 Jun 22, 2020
@remkonoteboom remkonoteboom linked an issue Jun 22, 2020 that may be closed by this pull request
@diegocortassa
Copy link
Contributor Author

Sorry I've been busy in the last few days and had no time to work on this, I'm adding a comment to the new issue because there is a reason for "mapping" to be both at root level and under "default" key

@diegocortassa diegocortassa deleted the DC-Patch057 branch August 4, 2020 23:51
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.

New style workflow > json issues.
3 participants