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

feat(caluma): Remove FormAnswer type #505

Merged

Conversation

open-dynaMIX
Copy link
Member

@open-dynaMIX open-dynaMIX commented Jun 14, 2019

There was a conceptual misunderstanding that led us to implement
nested answers. Which in turn created performance and complexity
issues.

This commit removes the FormAnswer type. Additionally also all
implementations of recursive lookups and form path handling (that only existed
as a result of this misconception) are removed.

There is a data migration that flattens the document tree.

Closes #504

BREAKING CHANGE:

  • The FormAnswer type is removed and answers to a question
    in a sub-form do not have their own document.
  • In order to access parent questions from within a table-sub-question
    JEXL, there is no need to prefix the question_slug with parent.
    anymore.
  • The JEXL expression rootForm is replaced by form.

@open-dynaMIX
Copy link
Member Author

/cc @czosel @winged @kaldras

Let's begin :)

I'm sure I've missed some things...

@czosel
Copy link
Contributor

czosel commented Jun 14, 2019

/cc @anehx

@open-dynaMIX open-dynaMIX force-pushed the refactor_nested_answers branch 9 times, most recently from e41d837 to be2c695 Compare June 20, 2019 14:18
@anehx
Copy link
Contributor

anehx commented Jun 20, 2019

❤️ 💯 🔥

@open-dynaMIX open-dynaMIX force-pushed the refactor_nested_answers branch 4 times, most recently from beb5e7c to 082a7fa Compare June 21, 2019 05:41
This was referenced Jun 21, 2019
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Nice work! Some clarifications / cleanups are needed however

caluma/form/serializers.py Show resolved Hide resolved
caluma/form/serializers.py Show resolved Hide resolved
caluma/form/validators.py Outdated Show resolved Hide resolved
caluma/form/validators.py Show resolved Hide resolved
There was a conceptual misunderstanding that led us to implement
nested answers. Which in turn created performance and complexity
issues.

This commit removes the FormAnswer type. Additionally also all
implementations of recursive lookups and form path handling (that only existed
as a result of this misconception) are removed.

There is a data migration that flattens the document tree.

Closes projectcaluma#504

BREAKING CHANGE:
 - The FormAnswer type is removed and answers to a question
   in a sub-form do not have their own document.
 - In order to access parent questions from within a table-sub-question
   JEXL, there is no need to prefix the question_slug with `parent.`
   anymore.
 - The JEXL expression `rootForm` is replaced by `form`.
@open-dynaMIX open-dynaMIX requested a review from winged June 24, 2019 12:11
@open-dynaMIX
Copy link
Member Author

@winged Ready for review!

Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Looks good now! 💯 👍

@open-dynaMIX open-dynaMIX merged commit 788406b into projectcaluma:master Jun 24, 2019
sliverc added a commit to sliverc/caluma that referenced this pull request Jul 30, 2019
* There is no need for answer_tree anymore as of projectcaluma#505
* parent doesn't exist and can be removed
* requiredness does not to be inherited as a form question does actually
  have answers.
sliverc added a commit to sliverc/caluma that referenced this pull request Jul 30, 2019
* There is no need for answer_tree anymore as of projectcaluma#505
* parent doesn't exist and can be removed
* requiredness does not to be inherited as a form question does actually
  have answers.
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.

Answers to FormQuestions must be flat
4 participants