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

Added very basic Open Forms integration #311

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

joeribekker
Copy link
Member

Very basic integration of Open Forms building on top of a detail page to show the form directly how it would show in Open Forms. This works with CSP headers and you can nicely select a form on the product admin page, once you configure Open Forms client in the admin.

Make sure you configure the environment variable "OPEN_FORMS_DOMAIN" to activate proper CSP headers. I didn't see any place to document this environment variable

Improvements can definitely be:

  • Tests that adding a form actually results the form page being rendered with the expected snippet.
  • Better page layout to show the full form.
  • Use of the future 0.1.1 library django-open-forms-client because now there is a default UUID in the migration that I don't want there.
  • Optionally, you might want to set the CSP-domein via database settings. We already add the domain in the OpenForm Configuration model, so might hook into that.

@joeribekker
Copy link
Member Author

That black failure is on the develop branch btw. but didn't want to touch it.

Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

  • As discussed with Joeri as well, this shoud not be merged until it is updated with the newer version of open-forms-client.
  • The domain (OPEN_FORMS_DOMAIN) should be added via an environmental variable for now.

{% openforms_sdk_media %}

{% endblock %}

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, the side bar {% block sidebar_content %} can be removed and add only a "back or cancel" button, but this can be done when we improve the layout generally.

@joeribekker joeribekker force-pushed the feature/open-forms-integration branch from 2b37ffa to f12b3fd Compare September 29, 2022 15:27
@joeribekker
Copy link
Member Author

Updated the lib and this PR. No default uuid in the migration needed anymore.

@joeribekker joeribekker requested a review from vaszig September 29, 2022 15:29
@joeribekker joeribekker force-pushed the feature/open-forms-integration branch from f12b3fd to bea93e9 Compare September 29, 2022 15:34
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

I think something goes wrong with some forms concerning the max length of the SlugField of Django. Is this happening to you as well?
image

@joeribekker
Copy link
Member Author

Weird, I thought slugfields had a length of 100? I'll check it out. I specifically used the same field as Open Forms to prevent exactly this problem, sigh :)

Make sure you configure the environment variable "OPEN_FORMS_DOMAIN"
to activate proper CSP headers.
@joeribekker joeribekker force-pushed the feature/open-forms-integration branch from bea93e9 to c0da313 Compare October 3, 2022 11:00
@joeribekker
Copy link
Member Author

Increased length to 100 chars. Note, you need to rollback and migrate again to make this effective.

@joeribekker joeribekker requested a review from vaszig October 3, 2022 14:46
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

Tested with the 0.2.2 newer version and it works fine!

@alextreme alextreme merged commit 17874bf into develop Oct 3, 2022
@alextreme alextreme deleted the feature/open-forms-integration branch October 3, 2022 15:27
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