Users are able to add more than one page to the same url

[Rafał Selewońko]

Users are able to add two pages to the same URL.

I think that model should cointain

unique_together = ("url", "sites")

but it's also not enough if you want to have some pages on more than one site.

[Joni]

Added patch to solve this issue with corresponding unittest and documentation.
The patch is located in the FlatPage ModelForm's clean() method, it checks that no other page within the same sites that the ModelForm's FlatPage instance belongs to has the same url.

[Graham King]

Fixed minor typo in warning message

[Graham King]

Yes it's a real problem, which can throw a MultipleObjectsReturned. Yes the patch applies cleanly on latest trunk and fixes the problem. Yes it has tests. Yes all the tests still pass with this patch applied.

It doesn't get much better than this.

[joni <joni@…>]

Glad to help :)

[Malcolm Tredinnick]

This is fixing the problem at the wrong level. It must be done through unique_together, since that will enforce it at the database level. After all, it's not compulsory to only add entries to that model's table through the admin interface.

We will add a release note patch for 1.4 (or next version after release) to indicate existing users might want to add this constraint to their database table. The main patch should be to add (and test) unique_together on the model and let the database enforce it and Django's normal form handling deal with errors.

[Kushagra Sinha]

Unique together does not work with many 2 many fields. https://code.djangoproject.com/ticket/702

I have thus added a m2m_changed listener in flatpage model which throws an exception on finding duplicate entries.

I think we need a design decision here as to whether this exception should be caught in admin site somehow or this ticket should be merged with https://code.djangoproject.com/ticket/702

[Kushagra Sinha]

Checks for duplicate flatpages and throws an exception

[Preston Holmes]

While unique together may not currently work with M2M - it could be said that this ticket is just a specific case of #702

Either way a signal is the wrong way to handle this, this should be done by overriding validate_unique in the FlatPage model, which could be done in this case because we do not need to solve unique_together for all cases of M2M. comment 9 in incorrect in that unique_together is not enforced at the database level (that is unique), and so the model validation code path is the only way to handle this.

A procedural note, the issue is a real issue, as such it is accepted. Design Decision Needed should, to quote the documentation: 'This stage is for issues which may be contentious, may be backwards incompatible, or otherwise involve high-level design decisions.' There is some nuance on when that test is met, but there is some push to reserve DDN only for those issues which truly need significant core braintrust to be involved.

[Kushagra Sinha]

Thanks for the note on Design Decision Needed, I'll keep it in mind.

As for overriding validate_unique - Correct me if I am wrong but it is called when the object is being saved and the m2m relations have not been calculated yet. Therefore if I override it, and check for say self.sites.all(), it will either throw an exception (in the case of new saves, saying you cannot access m2m fields when an object does not have an id) or will return the values before saving (if the object is being modified).

I can see of no way of getting the latest values in either an overridden save method or an overridden validate_unique method since both of them are called before/after saving the object but before calculating the m2m field.

They only way to read m2m fields before saving an object seems to be to hook onto m2m_changed

[Carl Meyer]

Replying to ptone:

While unique together may not currently work with M2M - it could be said that this ticket is just a specific case of #702

I've just converted #702 into a documentation ticket; I don't think generic support for M2Ms in unique_together is reasonable for the ORM.

Either way a signal is the wrong way to handle this, this should be done by overriding validate_unique in the FlatPage model, which could be done in this case because we do not need to solve unique_together for all cases of M2M. comment 9 in incorrect in that unique_together is not enforced at the database level (that is unique), and so the model validation code path is the only way to handle this.

unique_together is enforced at the database level - a UNIQUE constraint is created encompassing the unique_together fields. There is also code for unique_together in model validation, so it can be checked prior to save and raise a ValidationError instead of IntegrityError.

In any case, comment 9 is still inaccurate since m2m's can't go in unique_together. And j4nu5 is right that it can't go in the model validation code on the FlatPage model because m2ms haven't even been saved at that point. I think the options are either m2m_changed signal, or adding an explicit through model to the m2m and doing the validation there. In the end those will look pretty similar. In either case, the trick is finding a way that that error can be caught and presented nicely at the ModelForm layer (most importantly for the admin, but it should ideally not be implemented just for the admin), rather than just raising an IntegrityError. I don't think the current patch takes care of this.

[Kushagra Sinha]

Replying to carljm:

In any case, comment 9 is still inaccurate since m2m's can't go in unique_together. And j4nu5 is right that it can't go in the model validation code on the FlatPage model because m2ms haven't even been saved at that point. I think the options are either m2m_changed signal, or adding an explicit through model to the m2m and doing the validation there. In the end those will look pretty similar. In either case, the trick is finding a way that that error can be caught and presented nicely at the ModelForm layer (most importantly for the admin, but it should ideally not be implemented just for the admin), rather than just raising an IntegrityError. I don't think the current patch takes care of this.

I can catch the exception in Flatpage's ModelForm and display a message (from django.contrib) or something similar but the calling view is expecting the object to be successfully saved when it calls form's save method and thus, those view have to be made aware of the fact that an error may occur when form.save is called and therefore have to be patched.

Since the views have to be patched anyway, why don't we leave the error catching mechanism in ModelForm and instead put it the views which call form.save (add_view and change_view in admin specifically). We can then update the documentation stating that custom Flatpage views should expect IntegrityError when calling the save method. Its a bit ugly but prettier than other options. Tell me what you think.

[Carl Meyer]

Replying to j4nu5:

I can catch the exception in Flatpage's ModelForm and display a message (from django.contrib) or something similar but the calling view is expecting the object to be successfully saved when it calls form's save method and thus, those view have to be made aware of the fact that an error may occur when form.save is called and therefore have to be patched.

Since the views have to be patched anyway, why don't we leave the error catching mechanism in ModelForm and instead put it the views which call form.save (add_view and change_view in admin specifically). We can then update the documentation stating that custom Flatpage views should expect IntegrityError when calling the save method. Its a bit ugly but prettier than other options. Tell me what you think.

After poking around a bit, I think we'd be better off to pre-emptively trap the problem in the clean method of FlatpageForm, rather than trying to catch the IntegrityError after the fact. The latter requires special-casing in the view, which is always a last resort. Model-validation doesn't have access to m2m data, but a form's clean method does. The form should perhaps also be moved out of admin.py and into a new forms.py, so it's clearer that it's not just admin-specific but is useful for user code dealing with flatpages. And it should be documented.

And I think we should leave the current patch's signal-receiver in place that raises IntegrityError - that provides earlier warning for anyone writing code to save flatpages that doesn't go through the validation in FlatpageForm. (The documentation for FlatpageForm should mention that it validates this uniqueness requirement, and that any user code that saves flatpages should do so too or be prepared to catch IntegrityError).

As far as I can see, this is the best compromise we have available, since model-validation simply isn't able to handle validation of m2m fields (at least not in time to work nicely with a ModelForm).

[Carl Meyer]

Replying to carljm:

After poking around a bit, I think we'd be better off to pre-emptively trap the problem in the clean method of FlatpageForm, rather than trying to catch the IntegrityError after the fact. The latter requires special-casing in the view, which is always a last resort. Model-validation doesn't have access to m2m data, but a form's clean method does. The form should perhaps also be moved out of admin.py and into a new forms.py, so it's clearer that it's not just admin-specific but is useful for user code dealing with flatpages. And it should be documented.

And I think we should leave the current patch's signal-receiver in place that raises IntegrityError - that provides earlier warning for anyone writing code to save flatpages that doesn't go through the validation in FlatpageForm. (The documentation for FlatpageForm should mention that it validates this uniqueness requirement, and that any user code that saves flatpages should do so too or be prepared to catch IntegrityError).

As far as I can see, this is the best compromise we have available, since model-validation simply isn't able to handle validation of m2m fields (at least not in time to work nicely with a ModelForm).

I just realized that this proposal is essentially to combine the two patches that have already been proposed on this ticket.

[Carl Meyer]

After IRC discussion with Alex and James, the consensus is that the m2m_changed handler raising IntegrityError is overkill here, given the unlikelihood that anyone is actually doing anything with flatpages outside the admin, and the un-usefulness of raw IntegrityError that isn't handled by a validation step.

So the approach we'd like to take is to add the validation in the form's clean() method, move the form out into forms.py, document its existence and note the validation that it performs in case users are writing their own flatpage-creation or editing views.

This means the existing patch flatpages-ticket14678-2.diff attached by graham_king is close, it just needs the form moved out into forms.py and the documentation notes.

[Preston Holmes]

I'm assuming (on phone-can't verify) that tests are not passing with this as you are importing the form from old location in test

Small suggestion for docs is to replace "duplicate flatpages" with "duplicate flat page urls"

[Kushagra Sinha]

Replying to ptone:

I'm assuming (on phone-can't verify) that tests are not passing with this as you are importing the form from old location in test

Small suggestion for docs is to replace "duplicate flatpages" with "duplicate flat page urls"

Corrected the error. Improved the docs.

[Alex Gaynor]

a) This is a good use case for exists()

b) I'm pretty sure you can reduce this to a single SQL query for the case of "no duplicates" and only do O(n) to get the conflicting site if you need to generate an error message.

[Kushagra Sinha]

Uses ModelForm's overridden clean method. Has tests. Has docs.

[Carl Meyer]

In [16937]:

Fixed #14678 -- Added validation to catch flatpages with the same URL on the same site. Thanks seler for the report, and joni, graham_king, and j4nu5 for work on the patch.

[anonymous]

[16937] commit makes flatpages uneditable.

This is because saving modified flatpage is treated as creating new one with the same url.

[Carl Meyer]

This is a new issue; the issue reported in this ticket is fixed. Filed as #17057. Thanks for the report.

[Aymeric Augustin <aymeric.augustin@…>]

In e18968e0f221d2b7988d21aaec9684662cc14e4a:

Merge pull request #518 from vanschelven/patch-1

Fixed interpolation in a translated string.

Refs #14678.