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

IPublication behavior prevents save action if expiration date set before state change #325

Open
bizuyghur opened this issue Mar 17, 2021 · 6 comments

Comments

@bizuyghur
Copy link

One can not modify and save an item If item expiration date is set at the time of the creation and item state is changed to Published after the set expiration date.

The validator keep raising "Expiration date must be after publishing date." and save does not happen.

To reproduce:

Add a News item and set the Expiration date to past time. Save the item and change the State to Published. Go back and edit the News Item and Save. You will keep getting "Expiration date must be after publishing date" and can not save your changes.

version 2.6.5

@jensens
Copy link
Member

jensens commented Mar 18, 2021

Confirmed (here in Plone 6 coredev)!

This is valid for other content types with IPublication too.

On publication the publication date is set set automatically. It seems this does not check the expiration date. IIRC there's some subscriber handling this.

The "fun" part here is, even if this is wrong, afterwards there is no way to fix this in the edit form. If I set the publication date to a date before expiration date I still get the validation error.

Screenshot from 2021-03-18 09-48-50

@flipmcf
Copy link
Contributor

flipmcf commented Mar 22, 2021

There is not an easy fix for this yet.

I used IDublinCore behavior to reproduce.

My initial investigation shows that the invariant validator in IPublication is (incorrectly?) called many times, and a majority of those times without the IPublication fields and values from the request form.

Because the request form fields are not provided in some of these validations, the context is used and the values come from the object, not the form.

It only takes one invalid exception to cause a field validation error, and that's where it comes from.

For example, IDublinCore will come up on an iteration pass https://github.com/zopefoundation/z3c.form/blob/440eb18c541b88f64e88be9b66fbc27748f75af2/src/z3c/form/field.py#L208 and only use the Plone.basic behavior schema: title and description fields, then (incorrectly) call the IPublication invariant, resulting in a validation exception.

When the invariant is called correctly with the correct behavior schema, validation passes.

I need more experimentation to determine where the fix belongs, but I suspect it's in behavior registration or the 'bundling' of IDublinCore behaviors. I reserve the right to change my mind as I get more data.

@flipmcf
Copy link
Contributor

flipmcf commented Mar 23, 2021

A Content Type with only the behaviors [plone.basic + plone.categorization + plone.publication + plone.ownership] individually selected will not aggravate this bug.

However, a Content Type with plone.dublincore will aggravate this bug.

It's also quite obvious during debugging that z3c.form.field FieldWidget.validate() is called many more times when plone.dublincore is used, as opposed to the individual behaviors.

I am concerned this has exposed a deeper bug in form validation that may appear in other (Rare) cases where schemas use multiple inheritance

@provider(IFormFieldProvider)
class IDublinCore(IOwnership, IPublication, ICategorization, IBasic):
""" Metadata behavior providing all the DC fields
"""
pass
and invariants.
@invariant
def validate_start_end(data):
if data.effective and data.expires and data.effective > data.expires:

An invariant in one parent class (schema) is now called for every field from every parent class, but z3c form only provides the relevant fields request data defined for that specific parent class.

Another way to look at it: when we use multiple inheritance, the namespace of invariants gets lost.

Possible fixes:

  1. Accept this as a limitation in z3c.form, remove IDublinCore behavior from p.a.dexterity, and document the limitation.
  2. Fix z3c.form - providing all form fields to validation, and call the invariant validator multiple times unnecessarily, but correctly.
  3. Fix z3c.form - making it smarter about schema multiple inheritance and only calling invariants only when relevant to the field(s)
  4. Fix the symptom during the publication event, and prevent entering the state where effective > expires (kick the can down the road)
  5. Somehow, using python sorcery, (in p.a.dexterity) keep the parent schema's invariant (IPublication.validate_start_end) from being inherited into the child class namespace IDublinCore.validate_start_end, yet still gets called when the parent (IPublication) fields are validated - specifically avoiding changes in z3c.form

Any other ideas?

@jensens
Copy link
Member

jensens commented Mar 23, 2021

IDublinCore should go anyway. Its makes all so much more difficult. Probably something to consider for Plone 6.
For Plone 5 I would go for option 4. Option 3 is probably difficult to achieve.

@bizuyghur
Copy link
Author

content_status_modify at Products/CMFPlone/skins/plone_form_scripts/ sets the effective date when the state changed to Publish

@mauritsvanrees
Copy link
Member

This is still a problem in Plone 6.0.0b2.
Meanwhile the content_status_modify script has been turned into a browser view in plone.app.content.
Option 4 from above still seems like the way to go, at least in this beta stage where we do not want big changes anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants