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

Tests for tiles + support contentlistingtiles without collections #133

Merged
merged 32 commits into from
Jul 19, 2021

Conversation

djay
Copy link
Member

@djay djay commented Jun 18, 2021

This now makes the flow of creating a custom search page using mosaic much easier

  • edit mosaic page
  • add listingtile (if layout doesn't have by default)
  • add filter (should default to the right target css for ajax).
  • save.

It still supports using a filter tile targeting a collection but it's not clear how you would get the collection results onto the page when using ajax. You might still be able to use the "existing content" tile?

In addition this adds layer to test tiles. ie every robot test for portlets now becomes a test for tiles also. If you want to test just portlets or tiles use --layer tiles or --layer Portlet.

In addition this switches the settings to using the select2 widget. It's more tidy (and works in 5.0 when used in a tile).

Deco/Mosaic was designed to remove the need for collection objects completely. The contentlisting tile does everything a collection does (except syndication) so there isn't a need for it.

This does perhaps make the name collectionfilter confusing however. #135 aims to fix the names and simplify the settings panels.

Without further changes to contentlisting tile 5.0 is not supported when using tiles (because the change to standardtiles to allow this tile to listen to the request was made non compatible with 5.0 due to dependencies).

@djay
Copy link
Member Author

djay commented Jun 18, 2021

As part of this I want to simplify the wording and names to try and make things less confusing. esp the order of the fields in popups. Results Filter name might not be much better but Collection Filter isn't great either. Maybe Listing Filter or just Filter

Screen Shot 2021-06-19 at 12 15 15 am

@djay
Copy link
Member Author

djay commented Jun 21, 2021

The default content selector also needs to change.
Either tiles always default to .contentlisting-tile, or it defaults to blank and it will automatically work out the selector based on what its pointed at.

@djay djay mentioned this pull request Jun 21, 2021
@djay djay changed the title target contentlistingtiles or collections Tests for tiles + support contentlistingtiles without collections Jun 24, 2021
@djay djay requested a review from petschki June 24, 2021 05:38
@djay
Copy link
Member Author

djay commented Jun 24, 2021

@petschki The original reason for the PR was to have tiles tested so we can get other PRs merged to reduce our technical debt. If the inclusion of the contentlisting stuff is going to cause a delay I can potentially split the PR and change the tests to use collections.

Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

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

In general I like the approach and the simplification for mosaic filtering but right now, there's a hard dependency to plone.app.blocks which should not be in this package. Please refactor into the tiles folder which is only included when plone.tiles is installed.

src/collective/collectionfilter/filteritems.py Outdated Show resolved Hide resolved
src/collective/collectionfilter/filteritems.py Outdated Show resolved Hide resolved
@petschki
Copy link
Member

@petschki The original reason for the PR was to have tiles tested so we can get other PRs merged to reduce our technical debt. If the inclusion of the contentlisting stuff is going to cause a delay I can potentially split the PR and change the tests to use collections.

@djay that would be a great idea 👍🏼

@djay
Copy link
Member Author

djay commented Jun 24, 2021

@petschki The original reason for the PR was to have tiles tested so we can get other PRs merged to reduce our technical debt. If the inclusion of the contentlisting stuff is going to cause a delay I can potentially split the PR and change the tests to use collections.

@djay that would be a great idea 👍🏼

I'm saying I'd rather not because its extra work. Is the change I've made that big? if the current tests don't show it works then perhaps thats a bigger problem?

@djay djay requested a review from petschki June 24, 2021 06:42
@djay
Copy link
Member Author

djay commented Jun 24, 2021

@petschki done making changes.

@petschki
Copy link
Member

I just tried this with a buildout.coredev 5.2 and it works very nice. Since the tile_class is only available since plone.app.standardtiles==2.4.0 I've made a extra_install which sets the correct minimum version requirement. I've fixed also some minor typos in README.

But there's one issue I've had when testing it: If you enable Mosaic layout on a page and edit it the first time by adding a contentlisting tile and a filter tile, I got the following exception:

2021-06-25 08:19:19,961 ERROR   [Zope.SiteErrorLog:252][waitress-3] 1624601959.96020980.47787753061995 http://localhost:8080/Plone/filter-test/@@add-tile/collective.collectionfilter.tiles.filter
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 162, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 371, in publish_module
  Module ZPublisher.WSGIPublisher, line 274, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module plone.z3cform.layout, line 63, in __call__
  Module plone.z3cform.layout, line 47, in update
  Module plone.app.tiles.browser.base, line 104, in update
  Module plone.z3cform.fieldsets.extensible, line 65, in update
  Module plone.z3cform.patch, line 30, in GroupForm_update
  Module z3c.form.group, line 145, in update
  Module plone.app.z3cform.csrf, line 22, in execute
  Module z3c.form.action, line 98, in execute
  Module z3c.form.button, line 301, in __call__
  Module z3c.form.button, line 159, in __call__
  Module plone.app.tiles.browser.add, line 50, in handleAdd
  Module z3c.form.group, line 98, in extractData
  Module z3c.form.form, line 148, in extractData
  Module z3c.form.field, line 331, in extract
  Module z3c.form.field, line 303, in _extract
  Module collective.collectionfilter.validator, line 32, in validate
  Module zope.component._api, line 96, in queryAdapter
  Module zope.component.hooks, line 135, in adapter_hook
  Module collective.collectionfilter.tiles, line 68, in __init__
TypeError: No contentlisting tile or Collection found

This is due to the transient state of the mosaic page ... after you save the page with a contentlisting tile and edit it again you can add filter tiles as expected ... @djay I think this should be solved before we merge this.

@djay
Copy link
Member Author

djay commented Jun 25, 2021

@petschki hmm. I need to have some tests that run just for tiles only, so can test adding to a page instead of a collection.
Or maybe all tilel tests should be on a page.

This happens when you add a filter to a page before a content listing? or is the content listing not saved at all until teh page is saved?
It's possible we might have to live without validating the context is correct (ie has a content listing tiles) in the case its a mosaic page?

@petschki
Copy link
Member

This is a Page with mosaic enabled and edited the first time ... it makes no difference in which order you add filter/contentlisting tiles ... the filtertile always throws the exception, because https://github.com/collective/collective.collectionfilter/pull/133/files#diff-838484ed15774619566b2b294491ed8475f6d5047d9644d5a1297fed39965c9cR78 is always empty when adding a contentlisting tile the first time.

@djay
Copy link
Member Author

djay commented Jun 25, 2021

@petschki in that case I have to change the validation let you pick a collection or any ILayoutAware even if no listing tile.
So then what should happen? filter tiles show an error or just no options?

@petschki
Copy link
Member

Maybe remove the validator and show a portal message when the page was saved which says Please add at least one contentlisting tile to your page to show the results of the filters ... this could be done with an IObjectModified event listener. there might be only a javascript solution for validating the mandatory contentlisting tile during editing in the mosaic editor.

@djay
Copy link
Member Author

djay commented Jun 25, 2021 via email

@djay
Copy link
Member Author

djay commented Jun 28, 2021

@petschki is CF supposed to work if there is a collection anywhere in the acquaisition path? or only if it's in the direct context?
Seems like the the current validator allows it in aqusition but the other code doesn't?

@petschki
Copy link
Member

Only direct context makes sense to me ...

@djay
Copy link
Member Author

djay commented Jun 30, 2021

@petschki the problem with the tests is the one already identified and still not fixed :(
plone/plone.app.standardtiles#111
is 2.4.0 not supposed to be used with python 2.7?

@djay djay requested a review from petschki July 1, 2021 10:06
@djay
Copy link
Member Author

djay commented Jul 1, 2021

@petschki fixed the problem with the test setup. RichTextValue wasn't created correctly.
All tests are passing including the new one to check the warning message appears. Ready to test again

@petschki
Copy link
Member

petschki commented Jul 7, 2021

@djay I was able to test this locally and it works very well. Especially the automagical linking between filter and contentlisting is a huge improvement!

The only thing I found is, that when you save a page with filtertile and without contentlisting, the status message is shown correctly. When you edit and save the second time (also with filter and without contentlisting) the portal message doesn't show up anymore ...

also if you have a page with filter/contentlisting -> edit it and remove the contentlisting and save -> there is no status message/warning about missing contentlisting.

@djay
Copy link
Member Author

djay commented Jul 7, 2021

The only thing I found is, that when you save a page with filtertile and without contentlisting, the status message is shown correctly. When you edit and save the second time (also with filter and without contentlisting) the portal message doesn't show up anymore ...

Thats when you save the layout or save the filtertile? I only have an handler on the modify event for the filtertile but if you edit that again it should warn you again?

To have a more general warning system for editing anything on the page or just saving the whole layout then I'd need more clever code that finds all the possible filter tiles tests if they are ok. I'll have a think about it

@djay
Copy link
Member Author

djay commented Jul 12, 2021

@petschki it should now warn you whenever you save it. Please review.

Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

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

LGTM

@petschki
Copy link
Member

I'll merge this later today, I just want to test it more in a productive project of us.

@petschki
Copy link
Member

@djay just to be sure: the refactored UI of the tile-add-form isn't in this PR anymore, right? At least my add-forms are the same as before ...

@djay
Copy link
Member Author

djay commented Jul 13, 2021

@petschki correct. pretty sure all of that was in #135.

@djay
Copy link
Member Author

djay commented Jul 13, 2021

@petschki ah there was one UI change. I switched it to using the select2 widget. https://github.com/collective/collective.collectionfilter/pull/133/files#diff-4c74ac2fd432682735f055c860ac2e0dbc61b47acaed3ee94a1553660707f299
I can't remember exactly why but it was something to do with the robot tests and also because it looks a lot nicer IMO.

ah it was because the in and out widget breaks if used in tiles in plone 5.0.

Copy link
Member

@agitator agitator left a comment

Choose a reason for hiding this comment

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

lgtm

@djay
Copy link
Member Author

djay commented Jul 19, 2021

@agitator I merge or you?

@agitator agitator merged commit 5c1350d into master Jul 19, 2021
@agitator agitator deleted the collectionish branch July 19, 2021 11:46
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