-
Couldn't load subscription status.
- Fork 9
Global form submissions #76
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
base: main
Are you sure you want to change the base?
Conversation
… than re-computing everything
|
@mamico This is now working for us for our needs (as described in collective/volto-form-block#123). It is however a fairly big architectural change. Any comments on this approach? is it worth us finishing or there is no chance of getting this merged? |
@JeffersonBledsoe @djay I don't have a clear opinion, but at first glance, it does seem that handling the form definition and data in a centralized way aligns less with the Plone/Volto model. Have you considered an alternative like volto-repeatable-content-block, or maybe I didn't fully understand, or it doesn't entirely cover your use case? In any case, I would also check with @sneridagh and @robgietema, as they have been working extensively on the long-term rework of the product (collective/volto-form-block#110). |
|
The usecase is a form in a slot. Our slots editor allows this. For instance a below content slot or a form in a footer. It's not about what is the plone way to handle it. Is it a good idea to store form data in page and have that removed when the page is removed? Ir have the data not follow the form if it's moved from one page to another. I would argue not. I would argue that it's better to have a central control panel that lets you view the data collected by forms and remove or manage them with visibility. But in any case the current PR does not choose to change the default of storing data in the page if the page is available. It only stores it centrally if the page is not available such as the case with slots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some suggestions and thoughts
| self.request = request | ||
| self.form_data = self.extract_data_from_request() | ||
| self.block_id = self.form_data.get("block_id", "") | ||
| self.global_form_id = self.extract_data_from_request().get("global_form_id", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self.global_form_id = self.extract_data_from_request().get("global_form_id", "") | |
| self.global_form_id = self.form_data.get("global_form_id", "") |
| self.block = self.get_block_data(block_id=self.block_id) | ||
| self.block = self.get_block_data( | ||
| block_id=self.block_id, | ||
| global_form_id=self.form_data.get("global_form_id"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| global_form_id=self.form_data.get("global_form_id"), | |
| global_form_id=self.global_form_id, |
| block = self.get_block_data(block_id=form_data.get("block_id", "")) | ||
| block = self.get_block_data( | ||
| block_id=form_data.get("block_id", ""), | ||
| global_form_id=form_data.get("global_form_id"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| global_form_id=form_data.get("global_form_id"), | |
| global_form_id=self.global_form_id, |
| return form_data | ||
|
|
||
| def get_block_data(self, block_id): | ||
| def get_block_data(self, block_id, global_form_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably you don't need to pass global_form_id as parameter because is already set in init
| from zope.publisher.interfaces.browser import IBrowserRequest | ||
|
|
||
|
|
||
| GLOBAL_FORM_REGISTRY_RECORD_ID = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be a good idea moving this constant in a common place and import it also in post adapter?
| if value.get("send_message", ""): | ||
| transform = SafeHTML() | ||
| value["send_message"] = transform.scrub_html(value["send_message"]) | ||
| value = update_global_forms(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me better understand this..when you deserialize the block, you are storing its data twice: in the context and in registry, right?
So if the block is in a context, its data is duplicated? Or am i missing something?
I'm not a big fan of this approach, but i don't know if there is a better way to do it, or avoid setting it globally when we are working on a content item.
Or (maybe better): store form data always in registry, and in the content, only store the block_id reference. What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think storing both the saved data and the schema on the root object is the way to go, regardless of if there is a local page context or not. Then it's one implementation so less complicated.
This would mean that
- its possible to move a form between pages and keep the data
- it's possible have two forms that submit to the same savedata. For example a slightly altered one with extra info for logged in users
- it's possible to have a control panel that lets you view all saved data globally and remove it. giving some kind of audibility to collected data.
- but this does have implications for permissions. because then a private form on a private page is no longer private...
|
So how shall we proceed? |
For me, it could be finished and eventually merged into the main branch. |
This PR adds the ability for forms to be sent from outside the page they are on. The reasoning for this is so forms can be created elsewhere and inserted into a page, such as with a slots/ portlets mechanism.
This is achieved by creating a
global_formsregistry where forms are stored with aglobal_block_idwhich is generated on block save and saved with the block. If the client sends a form submission which matches against a global block id, that global form will be used.TODO