Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

ICEKit's Publishing status filters are not compatible with the Fluent Pages admin #32

Closed
mrmachine opened this issue Oct 7, 2016 · 7 comments
Assignees

Comments

@mrmachine
Copy link
Member

This manifests as a 500 error when attempting to filter the page change list.

Oddly, it appears that UrlNode itself is patched, but its default queryset is using an unpatched version or something.

In [1]: from django.utils.translation import activate

In [2]: activate(settings.LANGUAGE_CODE)

In [3]: UrlNode.objects.first().publishing_linked

In [4]: UrlNode.objects.filter(publishing_linked=None)
---------------------------------------------------------------------------
FieldError                                Traceback (most recent call last)
<ipython-input-4-b2e5f642c5be> in <module>()
----> 1 UrlNode.objects.filter(publishing_linked=None)

/usr/local/lib/python2.7/dist-packages/django/db/models/manager.pyc in manager_method(self, *args, **kwargs)
    125         def create_method(name, method):
    126             def manager_method(self, *args, **kwargs):
--> 127                 return getattr(self.get_queryset(), name)(*args, **kwargs)
    128             manager_method.__name__ = method.__name__
    129             manager_method.__doc__ = method.__doc__

/usr/local/lib/python2.7/dist-packages/django/db/models/query.pyc in filter(self, *args, **kwargs)
    677         set.
    678         """
--> 679         return self._filter_or_exclude(False, *args, **kwargs)
    680 
    681     def exclude(self, *args, **kwargs):

/usr/local/lib/python2.7/dist-packages/polymorphic/query.pyc in _filter_or_exclude(self, negate, *args, **kwargs)
    115         q_objects = translate_polymorphic_filter_definitions_in_args(self.model, args, using=self.db)  # the Q objects
    116         additional_args = translate_polymorphic_filter_definitions_in_kwargs(self.model, kwargs, using=self.db)  # filter_field='data'
--> 117         return super(PolymorphicQuerySet, self)._filter_or_exclude(negate, *(list(q_objects) + additional_args), **kwargs)
    118 
    119     def order_by(self, *args, **kwargs):

/usr/local/lib/python2.7/dist-packages/django/db/models/query.pyc in _filter_or_exclude(self, negate, *args, **kwargs)
    695             clone.query.add_q(~Q(*args, **kwargs))
    696         else:
--> 697             clone.query.add_q(Q(*args, **kwargs))
    698         return clone
    699 

/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.pyc in add_q(self, q_object)
   1308         existing_inner = set(
   1309             (a for a in self.alias_map if self.alias_map[a].join_type == INNER))
-> 1310         clause, require_inner = self._add_q(where_part, self.used_aliases)
   1311         self.where.add(clause, AND)
   1312         for hp in having_parts:

/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.pyc in _add_q(self, q_object, used_aliases, branch_negated, current_negated, allow_joins, split_subq)
   1336                     child, can_reuse=used_aliases, branch_negated=branch_negated,
   1337                     current_negated=current_negated, connector=connector,
-> 1338                     allow_joins=allow_joins, split_subq=split_subq,
   1339                 )
   1340                 joinpromoter.add_votes(needed_inner)

/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.pyc in build_filter(self, filter_expr, branch_negated, current_negated, can_reuse, connector, allow_joins, split_subq)
   1148         if not arg:
   1149             raise FieldError("Cannot parse keyword query %r" % arg)
-> 1150         lookups, parts, reffed_expression = self.solve_lookup_type(arg)
   1151         if not allow_joins and len(parts) > 1:
   1152             raise FieldError("Joined field references are not permitted in this query")

/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.pyc in solve_lookup_type(self, lookup)
   1034             if expression:
   1035                 return expression_lookups, (), expression
-> 1036         _, field, _, lookup_parts = self.names_to_path(lookup_splitted, self.get_meta())
   1037         field_parts = lookup_splitted[0:len(lookup_splitted) - len(lookup_parts)]
   1038         if len(lookup_parts) == 0:

/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.pyc in names_to_path(self, names, opts, allow_many, fail_on_missing)
   1395                     available = sorted(field_names + list(self.annotation_select))
   1396                     raise FieldError("Cannot resolve keyword %r into field. "
-> 1397                                      "Choices are: %s" % (name, ", ".join(available)))
   1398                 break
   1399             # Check if we need any joins for concrete inheritance cases (the

FieldError: Cannot resolve keyword 'publishing_linked' into field. Choices are: author, author_id, children, creation_date, id, in_navigation, in_sitemaps, key, layoutpage, level, lft, modification_date, parent, parent_id, parent_site, parent_site_id, polymorphic_ctype, polymorphic_ctype_id, publication_date, publication_end_date, redirectnode, rght, searchpage, status, translations, tree_id
sam-mi pushed a commit that referenced this issue Oct 7, 2016
 - add non-required FK to User on PublishingModel
 - update user on save (if user / called from admin)
 - add MyRecentModifications filter, not certain that it filters correctly
 because of this issue: #32
@jmurty
Copy link
Contributor

jmurty commented Oct 9, 2016

Hi @mrmachine can you talk more about what you need to achieve here?

ICEKit's Publishing implementation does not monkey-patch the UrlNode model to add the publishing_linked field (and DB column) to avoid us needing to import and manage Fluent Pages DB migrations in ICEKit applications. Only subclasses of our own PublishingModel get this field.

To filter generic UrlNode models by publishing status you cannot use the publishing_linked attribute, but you can instead use Fluent's UrlNode.status field (one of UrlNode.PUBLISHED or UrlNode.DRAFT) which we keep in sync with ICEKit's published status. This approach is already applied by the icekit.publishing.managers.UrlNodeQuerySetWithPublishingFeatures query set which is applied to UrlNode to provide our standard published()/draft()/visible() filters – though we may need to apply this query set more broadly?

Or better yet, you may be able to use one of ICEKit's existing publishing status filter implementations like icekit.publishing.admin.PublishingStatusFilter. We may need to adjust these to gracefully handle both Fluent's and ICEKit's notions of publishing, but if so doing this in a central location would be best.

@jmurty
Copy link
Contributor

jmurty commented Oct 9, 2016

The problem of filtering UrlNode items by publishing status – in a way that is consistent for both vanilla UrlNode and ICEKit's PublishingModel subclasses with their different publishing implementation – is solvable using just the UrlNode.status field I describe above.

However the original issue that triggered this ticket (Assembla ICEKit #151) needs deeper consideration which may make this particular issue irrelevant.

@mrmachine
Copy link
Member Author

This came up when attempting to implement assembla/icekit#151, but the problem exists on the develop branch with existing filters. I'm not sure if they ever worked, or if something has changed recently?

@jmurty
Copy link
Contributor

jmurty commented Oct 10, 2016

This is going to be a tricky problem to solve. The root problem is that we have two kinds of publishing systems in effect, Fluent's simple status boolean field approach and ICEKit's publishing_linked copy approach, which we need to handle somehow.

I see two possible approaches: Reconcile the divergent publishing approaches in the admin, or separate them.

  1. Reconcile the divergent publishing approaches as much as possible

This would involve modifying at least the publishing filters, and probably lower-level queryset logic, to allow us to filter on both PublishingModel and non-PublishingModel models and get consistent publishing status results across them to show in the admin.

This would allow the existing Fluent Page admin grouping at /admin/fluent_pages/page/ to work for both ICEKit publishing enabled models like LayoutPage etc, and Fluent Page derivatives that are not publishing-enabled like RedirectNode (why is this even a Page...?).

This is all possible since we have already done a lot of this kind of work to render published items on sites, but would likely be tricky work. We would need to further extend our publishing hacks to work in the admin context, in particular we would need to either resort to querying more directly on polymorphic child types instead of just Fluent UrlNode or Page items, at cost of poor performance, or find an alternative way of doing this same thing.

  1. Separate the divergent publishing approaches so PublishingModel-based items are shown separately from non-PublishingModel based items

This would route around the problem by showing ICEKit publishable items in a separate admin section from general Fluent pages, so our publishing-specific logic and queries will work without any hackery at the expense of having more endpoints in the admin for Fluent-derived models.

For this we would need to look into separating ICEKit publishing items from others in the admin, so instead showing all Fluent page derivatives at /admin/fluent_pages/page/ we might have two distinct admins for ICEKit Publishing items at /admin/fluent_pages/publishable_pages/ and non-publishing items at /admin/fluent_pages/unpublishable_pages/ -- or something like that.

I don't have a good sense for how feasible or difficult this might be, but in the simplest possible case (which might not work) we might be able to get away with a Fluent Page + PublishingModel abstract base class used solely as a hook to show ICEKit publishing items in a separate admin location, then hide these items from Fluent's default view.

@jmurty jmurty changed the title UrlNode queryset is not monkey patched properly for publishing. ICEKit's Publishing status filters are not compatible with the Fluent Pages admin Oct 10, 2016
@mrmachine
Copy link
Member Author

For option 2, there's still only one page tree. And icekit-publishable pages can be children to fluent pages. I don't see how we could effectively separate them into two pages in the admin.

Perhaps instead, we could display everything in the one page tree, but clearly indicate which things are icekit-publishable and which are not, and implement two distinct sets of filters for diverging fields?

This would expose some ugly and potentially confusing implementation details to users, e.g. that there are two types of publishing features.

Alternatively, we could insist that all enabled page plugins come from ICEkit and have ICEkit publishing features enabled. I think the only non-ICEkit page plugins we use are fairly trivial (e.g. redirectnode), and we would have to rewrite ICEkit versions of them. But we might want to do that anyway (at least for redirectnode).

@jmurty
Copy link
Contributor

jmurty commented Oct 10, 2016

I should note that my earlier suggestion to just filter by UrlNode.status won't actually work because ICEKit's publishing admin already applies a DRAFT-only filter to items shown in the admin, which means the filters will never see items that have been ICEKit-published. This can be undone or worked-around, but things quickly start to get complicated.

jmurty added a commit that referenced this issue Oct 10, 2016
Make the `PublishingPublishedFilter` and `PublishingStatusFilter`
publishing status admin filters compatible with models that do not
provide the standard ICEKit publishing features (and DB fields) from
`PublishingModel`.

This change is necessary to make these filters usable on parent admin
pages that can include non-`PublishingModel` class, in particular the
Fluent Pages default parent page admin listing /admin/fluent_pages/page/

This change is a hack and requires iterating over the actual instances
of all models so it will be slow in cases where we cannot rely on
`PublishingModel` fields being present, but works around the problem
for now of filtering by two different publishing notions in the one
polymorphic parent admin page.
cogat added a commit that referenced this issue Nov 21, 2016
…s-urlnode-compatible

Make publishing filters work with generic Fluent models, re #32
@jmurty
Copy link
Contributor

jmurty commented Nov 23, 2016

This issue was fixed by commit 8951966 which was recently merged into develop in 09b193d

@jmurty jmurty closed this as completed Nov 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants