feat(workflow): implement WorkItem redo pattern #1656

open-dynaMIX · 2022-01-13T10:59:27Z

This commit implements a possibility to redo an already finished
WorkItem and all the ones that were finished in between.

Closes #1510

winged

Code looks great! However I do have a couple of issues that need to be addressed.

Additionally to the things noted in the code, I feel like this feature needs some more documentation. In the Markdowns as well as in the code

winged · 2022-01-17T09:36:59Z

caluma/caluma_workflow/models.py

@@ -253,6 +260,27 @@ class WorkItem(UUIDModel):
        null=True,
    )

+    def get_redoable(self):
+        jexl = None
+        if self.previous_work_item:


Why are we looking at the previous_work_item's redoable field instead of "our own" here?

Also: if you'd invert the if condition, you could simplify the code by using an early return

Why are we looking at the previous_work_item's redoable field instead of "our own" here?

Because there we have the redoable field. If we would set it on our own task_flow, we wouldn't be able to redo a workitem if we reached one without a task_flow (the last one in a branch).

Also: if you'd invert the if condition, you could simplify the code by using an early return

Done

Why are we looking at the previous_work_item's redoable field instead of "our own" here?

Because there we have the redoable field. If we would set it on our own task_flow, we wouldn't be able to redo a workitem if we reached one without a task_flow (the last one in a branch).

Aah, that makes sense. However, I'm really not too happy about that. I think this really should be directly on the affected work item ('s task flow) instead. The mental indirection required sounds rather tricky IMO. Consider that a TaskFlow tells us "where to go from here" (flow.next), I would strongly expect the redo to tellus "where to go back from here", not "where to go back from the next item".

This unfortunately sounds like a bit more work, but I think we should store the "redo" data local to where it's relevant, not "on the previous task's task flow".

One solution I think would be acceptable for this is to make the flow foreign key nullable, so we can have a task flow that doesn't lead anywhere but may allow redoing. In the long term, as discussed, the Flow may even be dropped alltogether

caluma/caluma_workflow/domain_logic.py

winged · 2022-01-17T09:46:07Z

caluma/caluma_workflow/domain_logic.py

+            wi.save()
+
+    @classmethod
+    def _find_ready_in_work_item_tree(cls, work_item, allowed_states=None, ready=None):


If you'd convert this method into a generator, you'll improve two thing: The code gets simpler as you can use yield from instead of building up a list and passing it along. And also, it could improve performance, as we won't have to walk the tree further than needed once a workitem is found that allows the redo

Nice, done.

caluma/caluma_workflow/domain_logic.py

This commit implements a possibility to redo an already finished WorkItem and all the ones that were finished in between. Closes projectcaluma#1510

winged

Awesome, I like it much better this way 🎉

We're still lacking documentation, but as we discussed in another place, we should overhaul it anyway, and we can add that bit then.

* feat(form): enable hint messages on questions (#1655) Add a hint text field on all question types except for form, static and action button questions. This functionality requires a db migration. * feat(workflow): implement WorkItem redo pattern (#1656) This commit implements a possibility to redo an already finished WorkItem and all the ones that were finished in between. Closes #1510 * chore: update dependencies (#1660) * chore: cleanup `WorkItem.get_redoable()` * chore: update dependencies * chore: release 7.15 (#1661) * chore: add a code of conduct (#1651) * chore(deps): update graphene-django Update to graphene-django 2.12.1 and fix connection field, schema. * chore(deps): add pytest-xfaillist plugin * chore(deps): switch to graphql 3.0 * chore: fix ResolveInfo fixture * chore(test): ensure name is valid identifier * chore(deps): remove XPASSing tests from xfails * chore(deps): handle enum in JSONValueFilter * chore(deps): remove passing tests from xfails.list * fix(tests): hand over UUID as string * chore(deps): get ordering value from Enum if any * chore(deps): use graphene's updated data types issue #814 * chore(deps): update and sort xfaillist * chore(deps): remove passing test from xfaillist #814 * chore(deps): handle enum in filter #814 * check if these tests are correct if so fixup commit with previous * chore(deps): upgrade pytest-xfaillist * chore(deps): pass kwargs to data source resolver (#1643) * fix(graphene): make more tests work (#1644) * Introduce our own choices field (for now): Graphene seems to already instantiate Enum instances from the request data, which causes the regular DRF choice field to fail, as the enum object isn't handled * Fix a few resolvers which now receive additional (unused by use) parameters * Graphene django v3 (#1645) * fix(datasource): accept new additional parameters from graphene * fix(filters): overload resolve_queryset() to support multi order_by Graphene does not really support order_by with multiple values like we need it. There's a bug in graphene that we need to work around: graphql-python/graphene-django#1280 While we're at it, also clean up some more data. * fix(historical): fix uuid and resolver params (#1646) * fix(historical): fix uuid and resolver params * fix(form): accept new additional resolver args * Graphene django v3 (#1647) * fix(tests): adjust test to new error message style graphene 3.0 returns a text fragment of the offending query * fix(middleware): use new visitor and ast conventions The node types are not CamelCase named, but snake_cased. Also, the operation is an AST object, not a bare string. In queries without variables, the variables MUST NOT be set to an empty string. (None works however...) * fix(cases): various fixes in csae filter and tests * fix(schema): make schema tests work again (sort of) The schema test relies on stringification of the root schema object. This currently fails due to a function within the graphql core package being unable to handle django lazy translation objects. The test setup is rigged such that once upstream fixes the problem, it will trigger a failure here and we can remove the hacky workaround. * fix(tests): more tests fixed with correct (non-uuid) input objects Also remove passing some info objects to schema_executor(), it doesn't accept them anymore (just pass context_value as that's the interesting bit) * fix(tests): stringify localized fields when passing to gql * chore(coverage): improve coverage Most of the lines that were "not covered" will never happen or are error cases such as raising validation errors, or other non-problematic parts. * fix(tests): sort the schema type map (#1648) A sorted type map (as it used to be in older graphene versions) helps better comparing / diffing the schema * Graphene django v3 (#1649) * chore: coverage back to 100% * chore: fix conflict in setup.py * fix(schema): use explicit types for case status and workitem status Graphene would otherwise use an automatic naming scheme that breaks our clients * fix: coverage in new status fields (#1650) Those status fields are only used for the type system, but not as actual input types. Thus, they're never actually instantiated * fix: pin graphql-relay to <3.1.1 the minor version includes deprecation of backwards compatibility measures. See graphql-python/graphql-relay-py#45 Might include other changes that are unrelated to the above that break a number of our tests. * update snapshots for test_question * fix(test): fix UUID * fix(tests): update test_workflow snapshot * fix: remove schema workaround and update snapshot Co-authored-by: luytena <[email protected]> Co-authored-by: Fabio Ambauen <[email protected]> Co-authored-by: David Vogt <[email protected]> Co-authored-by: Christian Zosel <[email protected]> Co-authored-by: Stefan Borer <[email protected]> Co-authored-by: Jean-Louis Fuchs <[email protected]>

* chore(deps): update graphene-django Update to graphene-django 2.12.1 and fix connection field, schema. * chore(deps): add pytest-xfaillist plugin * chore(deps): switch to graphql 3.0 * chore: fix ResolveInfo fixture * chore(test): ensure name is valid identifier * chore(deps): remove XPASSing tests from xfails * chore(deps): handle enum in JSONValueFilter * chore(deps): remove passing tests from xfails.list * fix(tests): hand over UUID as string * chore(deps): get ordering value from Enum if any * chore(deps): use graphene's updated data types issue #814 * chore(deps): update and sort xfaillist * chore(deps): remove passing test from xfaillist #814 * chore(deps): handle enum in filter #814 * check if these tests are correct if so fixup commit with previous * chore(deps): upgrade pytest-xfaillist * chore(deps): pass kwargs to data source resolver (#1643) * fix(graphene): make more tests work (#1644) * Introduce our own choices field (for now): Graphene seems to already instantiate Enum instances from the request data, which causes the regular DRF choice field to fail, as the enum object isn't handled * Fix a few resolvers which now receive additional (unused by use) parameters * Graphene django v3 (#1645) * fix(datasource): accept new additional parameters from graphene * fix(filters): overload resolve_queryset() to support multi order_by Graphene does not really support order_by with multiple values like we need it. There's a bug in graphene that we need to work around: graphql-python/graphene-django#1280 While we're at it, also clean up some more data. * fix(historical): fix uuid and resolver params (#1646) * fix(historical): fix uuid and resolver params * fix(form): accept new additional resolver args * Graphene django v3 (#1647) * fix(tests): adjust test to new error message style graphene 3.0 returns a text fragment of the offending query * fix(middleware): use new visitor and ast conventions The node types are not CamelCase named, but snake_cased. Also, the operation is an AST object, not a bare string. In queries without variables, the variables MUST NOT be set to an empty string. (None works however...) * fix(cases): various fixes in csae filter and tests * fix(schema): make schema tests work again (sort of) The schema test relies on stringification of the root schema object. This currently fails due to a function within the graphql core package being unable to handle django lazy translation objects. The test setup is rigged such that once upstream fixes the problem, it will trigger a failure here and we can remove the hacky workaround. * fix(tests): more tests fixed with correct (non-uuid) input objects Also remove passing some info objects to schema_executor(), it doesn't accept them anymore (just pass context_value as that's the interesting bit) * fix(tests): stringify localized fields when passing to gql * chore(coverage): improve coverage Most of the lines that were "not covered" will never happen or are error cases such as raising validation errors, or other non-problematic parts. * fix(tests): sort the schema type map (#1648) A sorted type map (as it used to be in older graphene versions) helps better comparing / diffing the schema * Graphene django v3 (#1649) * chore: coverage back to 100% * chore: fix conflict in setup.py * fix(schema): use explicit types for case status and workitem status Graphene would otherwise use an automatic naming scheme that breaks our clients * fix: coverage in new status fields (#1650) Those status fields are only used for the type system, but not as actual input types. Thus, they're never actually instantiated * rebase graphene update branch on main (#1664) * feat(form): enable hint messages on questions (#1655) Add a hint text field on all question types except for form, static and action button questions. This functionality requires a db migration. * feat(workflow): implement WorkItem redo pattern (#1656) This commit implements a possibility to redo an already finished WorkItem and all the ones that were finished in between. Closes #1510 * chore: update dependencies (#1660) * chore: cleanup `WorkItem.get_redoable()` * chore: update dependencies * chore: release 7.15 (#1661) * chore: add a code of conduct (#1651) * chore(deps): update graphene-django Update to graphene-django 2.12.1 and fix connection field, schema. * chore(deps): add pytest-xfaillist plugin * chore(deps): switch to graphql 3.0 * chore: fix ResolveInfo fixture * chore(test): ensure name is valid identifier * chore(deps): remove XPASSing tests from xfails * chore(deps): handle enum in JSONValueFilter * chore(deps): remove passing tests from xfails.list * fix(tests): hand over UUID as string * chore(deps): get ordering value from Enum if any * chore(deps): use graphene's updated data types issue #814 * chore(deps): update and sort xfaillist * chore(deps): remove passing test from xfaillist #814 * chore(deps): handle enum in filter #814 * check if these tests are correct if so fixup commit with previous * chore(deps): upgrade pytest-xfaillist * chore(deps): pass kwargs to data source resolver (#1643) * fix(graphene): make more tests work (#1644) * Introduce our own choices field (for now): Graphene seems to already instantiate Enum instances from the request data, which causes the regular DRF choice field to fail, as the enum object isn't handled * Fix a few resolvers which now receive additional (unused by use) parameters * Graphene django v3 (#1645) * fix(datasource): accept new additional parameters from graphene * fix(filters): overload resolve_queryset() to support multi order_by Graphene does not really support order_by with multiple values like we need it. There's a bug in graphene that we need to work around: graphql-python/graphene-django#1280 While we're at it, also clean up some more data. * fix(historical): fix uuid and resolver params (#1646) * fix(historical): fix uuid and resolver params * fix(form): accept new additional resolver args * Graphene django v3 (#1647) * fix(tests): adjust test to new error message style graphene 3.0 returns a text fragment of the offending query * fix(middleware): use new visitor and ast conventions The node types are not CamelCase named, but snake_cased. Also, the operation is an AST object, not a bare string. In queries without variables, the variables MUST NOT be set to an empty string. (None works however...) * fix(cases): various fixes in csae filter and tests * fix(schema): make schema tests work again (sort of) The schema test relies on stringification of the root schema object. This currently fails due to a function within the graphql core package being unable to handle django lazy translation objects. The test setup is rigged such that once upstream fixes the problem, it will trigger a failure here and we can remove the hacky workaround. * fix(tests): more tests fixed with correct (non-uuid) input objects Also remove passing some info objects to schema_executor(), it doesn't accept them anymore (just pass context_value as that's the interesting bit) * fix(tests): stringify localized fields when passing to gql * chore(coverage): improve coverage Most of the lines that were "not covered" will never happen or are error cases such as raising validation errors, or other non-problematic parts. * fix(tests): sort the schema type map (#1648) A sorted type map (as it used to be in older graphene versions) helps better comparing / diffing the schema * Graphene django v3 (#1649) * chore: coverage back to 100% * chore: fix conflict in setup.py * fix(schema): use explicit types for case status and workitem status Graphene would otherwise use an automatic naming scheme that breaks our clients * fix: coverage in new status fields (#1650) Those status fields are only used for the type system, but not as actual input types. Thus, they're never actually instantiated * fix: pin graphql-relay to <3.1.1 the minor version includes deprecation of backwards compatibility measures. See graphql-python/graphql-relay-py#45 Might include other changes that are unrelated to the above that break a number of our tests. * update snapshots for test_question * fix(test): fix UUID * fix(tests): update test_workflow snapshot * fix: remove schema workaround and update snapshot Co-authored-by: luytena <[email protected]> Co-authored-by: Fabio Ambauen <[email protected]> Co-authored-by: David Vogt <[email protected]> Co-authored-by: Christian Zosel <[email protected]> Co-authored-by: Stefan Borer <[email protected]> Co-authored-by: Jean-Louis Fuchs <[email protected]> * chore: cleanup schema test workaround * fix(coverage): raise cov and fix a typo * chore(deps): update graphene-django * chore(deps): Update to graphene-django 2.12.1 and fix connection field, schema. * chore(deps): add pytest-xfaillist plugin * chore(deps): switch to graphql 3.0 * chore: fix ResolveInfo fixture * chore(test): ensure name is valid identifier * chore(deps): handle enum in JSONValueFilter * fix(tests): hand over UUID as string * chore(deps): get ordering value from Enum if any * chore(deps): use graphene's updated data types issue #814 * chore(deps): handle enum in filter #814 * chore(deps): pass kwargs to data source resolver (#1643) * fix(graphene): make more tests work (#1644) * Introduce our own choices field (for now): Graphene seems to already instantiate Enum instances from the request data, which causes the regular DRF choice field to fail, as the enum object isn't handled * Fix a few resolvers which now receive additional (unused by use) parameters * Graphene django v3 (#1645) * fix(datasource): accept new additional parameters from graphene * fix(filters): overload resolve_queryset() to support multi order_by Graphene does not really support order_by with multiple values like we need it. There's a bug in graphene that we need to work around: graphql-python/graphene-django#1280 While we're at it, also clean up some more data. * fix(historical): fix uuid and resolver params (#1646) * fix(historical): fix uuid and resolver params * fix(form): accept new additional resolver args * Graphene django v3 (#1647) * fix(tests): adjust test to new error message style graphene 3.0 returns a text fragment of the offending query * fix(middleware): use new visitor and ast conventions The node types are not CamelCase named, but snake_cased. Also, the operation is an AST object, not a bare string. In queries without variables, the variables MUST NOT be set to an empty string. (None works however...) * fix(cases): various fixes in csae filter and tests * fix(schema): make schema tests work again (sort of) The schema test relies on stringification of the root schema object. This currently fails due to a function within the graphql core package being unable to handle django lazy translation objects. The test setup is rigged such that once upstream fixes the problem, it will trigger a failure here and we can remove the hacky workaround. * fix(tests): more tests fixed with correct (non-uuid) input objects Also remove passing some info objects to schema_executor(), it doesn't accept them anymore (just pass context_value as that's the interesting bit) * fix(tests): stringify localized fields when passing to gql * chore(coverage): improve coverage Most of the lines that were "not covered" will never happen or are error cases such as raising validation errors, or other non-problematic parts. * fix(tests): sort the schema type map (#1648) A sorted type map (as it used to be in older graphene versions) helps better comparing / diffing the schema * Graphene django v3 (#1649) * chore: coverage back to 100% * chore: fix conflict in setup.py * fix(schema): use explicit types for case status and workitem status Graphene would otherwise use an automatic naming scheme that breaks our clients * fix: coverage in new status fields (#1650) Those status fields are only used for the type system, but not as actual input types. Thus, they're never actually instantiated * rebase graphene update branch on main (#1664) * feat(form): enable hint messages on questions (#1655) Add a hint text field on all question types except for form, static and action button questions. This functionality requires a db migration. * feat(workflow): implement WorkItem redo pattern (#1656) This commit implements a possibility to redo an already finished WorkItem and all the ones that were finished in between. Closes #1510 * chore: update dependencies (#1660) * chore: cleanup `WorkItem.get_redoable()` * chore: update dependencies * chore: release 7.15 (#1661) * chore: add a code of conduct (#1651) * chore(deps): update graphene-django Update to graphene-django 2.12.1 and fix connection field, schema. * chore(deps): add pytest-xfaillist plugin * chore(deps): switch to graphql 3.0 * chore: fix ResolveInfo fixture * chore(test): ensure name is valid identifier * chore(deps): remove XPASSing tests from xfails * chore(deps): handle enum in JSONValueFilter * chore(deps): remove passing tests from xfails.list * fix(tests): hand over UUID as string * chore(deps): get ordering value from Enum if any * chore(deps): use graphene's updated data types issue #814 * chore(deps): update and sort xfaillist * chore(deps): remove passing test from xfaillist #814 * chore(deps): handle enum in filter #814 * check if these tests are correct if so fixup commit with previous * chore(deps): upgrade pytest-xfaillist * chore(deps): pass kwargs to data source resolver (#1643) * fix(graphene): make more tests work (#1644) * Introduce our own choices field (for now): Graphene seems to already instantiate Enum instances from the request data, which causes the regular DRF choice field to fail, as the enum object isn't handled * Fix a few resolvers which now receive additional (unused by use) parameters * Graphene django v3 (#1645) * fix(datasource): accept new additional parameters from graphene * fix(filters): overload resolve_queryset() to support multi order_by Graphene does not really support order_by with multiple values like we need it. There's a bug in graphene that we need to work around: graphql-python/graphene-django#1280 While we're at it, also clean up some more data. * fix(historical): fix uuid and resolver params (#1646) * fix(historical): fix uuid and resolver params * fix(form): accept new additional resolver args * Graphene django v3 (#1647) * fix(tests): adjust test to new error message style graphene 3.0 returns a text fragment of the offending query * fix(middleware): use new visitor and ast conventions The node types are not CamelCase named, but snake_cased. Also, the operation is an AST object, not a bare string. In queries without variables, the variables MUST NOT be set to an empty string. (None works however...) * fix(cases): various fixes in csae filter and tests * fix(schema): make schema tests work again (sort of) The schema test relies on stringification of the root schema object. This currently fails due to a function within the graphql core package being unable to handle django lazy translation objects. The test setup is rigged such that once upstream fixes the problem, it will trigger a failure here and we can remove the hacky workaround. * fix(tests): more tests fixed with correct (non-uuid) input objects Also remove passing some info objects to schema_executor(), it doesn't accept them anymore (just pass context_value as that's the interesting bit) * fix(tests): stringify localized fields when passing to gql * chore(coverage): improve coverage Most of the lines that were "not covered" will never happen or are error cases such as raising validation errors, or other non-problematic parts. * fix(tests): sort the schema type map (#1648) A sorted type map (as it used to be in older graphene versions) helps better comparing / diffing the schema * Graphene django v3 (#1649) * chore: coverage back to 100% * chore: fix conflict in setup.py * fix(schema): use explicit types for case status and workitem status Graphene would otherwise use an automatic naming scheme that breaks our clients * fix: coverage in new status fields (#1650) Those status fields are only used for the type system, but not as actual input types. Thus, they're never actually instantiated * fix: pin graphql-relay to <3.1.1 the minor version includes deprecation of backwards compatibility measures. See graphql-python/graphql-relay-py#45 Might include other changes that are unrelated to the above that break a number of our tests. * update snapshots for test_question * fix(test): fix UUID * fix(tests): update test_workflow snapshot * fix: remove schema workaround and update snapshot Co-authored-by: luytena <[email protected]> Co-authored-by: Fabio Ambauen <[email protected]> Co-authored-by: David Vogt <[email protected]> Co-authored-by: Christian Zosel <[email protected]> Co-authored-by: Stefan Borer <[email protected]> Co-authored-by: Jean-Louis Fuchs <[email protected]> * chore: cleanup schema test workaround * feat(form): enable hint messages on questions (#1655) Add a hint text field on all question types except for form, static and action button questions. This functionality requires a db migration. * feat(workflow): implement WorkItem redo pattern (#1656) This commit implements a possibility to redo an already finished WorkItem and all the ones that were finished in between. Closes #1510 * fix(coverage): raise cov and fix a typo * fixme: question snapshot differs * fixme: caluma analytics snapshot differs and tests fail * fix: incomplete merge * chore(analytics): compatibility cleanups Co-authored-by: Stefan Borer <[email protected]> Co-authored-by: Jean-Louis Fuchs <[email protected]> Co-authored-by: David Vogt <[email protected]> Co-authored-by: luytena <[email protected]> Co-authored-by: Fabio Ambauen <[email protected]> Co-authored-by: Christian Zosel <[email protected]>

open-dynaMIX force-pushed the workitem_redo branch 5 times, most recently from 58e572c to cd8ff94 Compare January 17, 2022 08:12

open-dynaMIX marked this pull request as ready for review January 17, 2022 08:37

open-dynaMIX requested review from winged and anehx January 17, 2022 08:37

winged requested changes Jan 17, 2022

View reviewed changes

open-dynaMIX force-pushed the workitem_redo branch 2 times, most recently from 094d980 to 341158c Compare January 17, 2022 11:16

open-dynaMIX requested a review from winged January 17, 2022 11:29

feat(workflow): implement WorkItem redo pattern

61b670b

This commit implements a possibility to redo an already finished WorkItem and all the ones that were finished in between. Closes projectcaluma#1510

open-dynaMIX force-pushed the workitem_redo branch from 341158c to 61b670b Compare January 17, 2022 12:53

winged approved these changes Jan 17, 2022

View reviewed changes

open-dynaMIX merged commit d01e284 into projectcaluma:main Jan 17, 2022

open-dynaMIX deleted the workitem_redo branch January 17, 2022 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(workflow): implement WorkItem redo pattern #1656

feat(workflow): implement WorkItem redo pattern #1656

open-dynaMIX commented Jan 13, 2022

winged left a comment

winged Jan 17, 2022

open-dynaMIX Jan 17, 2022

winged Jan 17, 2022

open-dynaMIX Jan 17, 2022

winged Jan 17, 2022

open-dynaMIX Jan 17, 2022

winged left a comment

feat(workflow): implement WorkItem redo pattern #1656

feat(workflow): implement WorkItem redo pattern #1656

Conversation

open-dynaMIX commented Jan 13, 2022

winged left a comment

Choose a reason for hiding this comment

winged Jan 17, 2022

Choose a reason for hiding this comment

open-dynaMIX Jan 17, 2022

Choose a reason for hiding this comment

winged Jan 17, 2022

Choose a reason for hiding this comment

open-dynaMIX Jan 17, 2022

Choose a reason for hiding this comment

winged Jan 17, 2022

Choose a reason for hiding this comment

open-dynaMIX Jan 17, 2022

Choose a reason for hiding this comment

winged left a comment

Choose a reason for hiding this comment