Products.cmfplone: PLIP: Cleanup and enhance icon and thumb aspects

Created on 30 Aug 2016 · 98Comments · Source: plone/Products.CMFPlone

Proposer : fgrcon

Seconder : Jens W. Klein

Abstract

Pre plone 5 icons to decorate content types were implemented using tags. Since plone 5 these image tags have been replaced by fontello fonts in css rules (:before{content=e80...} ). Furthermore mimetype icons where used for file types such as *.pdf, *.xls and many more until plone 4.x. - in plone 5 all those are replaced by a single fontello font (paper clip).

There has been some confusion about icons and preview images (aka thumbs). In summary views thumbs were shown for objects containing a lead image. In plone 5.02 the option to show thumbs in further list and table views, folder content listing , portlets etc. was added.

Configurability of this feature (where to show thumbs, where to suppress them, individual thumb size ) is insufficient (only never, authenticated users only, always).

The implementation of this feature reuses an obsolete meta data catalog item: getIcon (now bool: True when item is an image cor contains an image (e.g. lead image) - this adds to the confusion between thumbs and icons enormously.

Concise but sufficient documentation (user and developer) is to be provided.

Motivation

Cleanup, better usabilty, make life easier for users, integrators, developers.
get same behavior, look and feel allover the framework

Assumptions

see abstract

Proposal & Implementation

~~- [ ] change meta-data name getIcon to hasImage (keep getIcon with deprecation warning for a while ?)~~
This should be moved to an independent PLIP beacause it needs further discussion. Renaming this metadata item might generate unacceptable effort for reindexing or cause problems for add ons.
Maybe just to be solved with explanations in documentation.

[x] provide more detailed options in Site Control panel (Suppress thumbs in list views, table views, summary view, portlets, default thumbsizes for list-, table-, summary- views and portlets.
[x] Additional behavior IThumbIconHandling (in p.a.contenttypes) for dexterity types, which allow to override thumb and icon options for folders and collections and custom types.
~~This Behavior is added to folder and collection by default.~~ (see comment by @ebrehault below, explanations will go into documentation ....)
[x] make thumbsize configurable in folder contents (structure pattern)
[x] add override thumb and icon options to all standard portlets in plone.app.portlets, ...collection, ...event :
- [x] navigation
- [x] news
- [x] revision
- [x] recent
- [x] plone.portlet.collection
- [x] plone.app.event

for all listed portlets:
nav-portlet

[x] remove fontello font (paper clip) for file types and replace with mime type icon (to many mimetypes to create fontello fonts for each of them)

before e.g.:
tableview-before

after:
tableview-after

[x] documentation for all stakeholders (covering metadata, icons, thumb aspects)
WIP will be submitted after prs are merged
[x] upgradesteps to reapply profiles ( new Registry records, ~~add IThumbHandling behavior to dx folder and collection~~ )
[x] plone-compile-resources
some additional translations

remove dead code as much as possible :

[x] plone.app.layout
[x] Products.CMFPlone

Deliverables

finished:

Products.CMFPlone
plone.app.content
mockup (structure)
plone.app.contentlisting
plone.app.contenttypes
plone.app.layout
plone.app.portlets
plone.portlet.collection
plone.app.event
plone.documentation
plone.theme.barceloneta
plone.app.upgrade

Risks

~~- renaming getIcon might cause troubles for addons~~
postponed see above..

some test cases might need rework

Participants

fgrcon

most of the neccessary modifications are already implemented and tested in private environment. Pull requests will follow shortly.

Related issues

Thumbs visibility: shown in too much listings https://github.com/plone/Products.CMFPlone/issues/1322
toggle preview images in folder/collection listings on per context basis https://github.com/plone/Products.CMFPlone/issues/1185
Collection views do not display lead images set on a content type https://github.com/plone/Products.CMFPlone/issues/1185 (closed)
Cleanup deprecated geticon ... code https://github.com/plone/Products.CMFPlone/issues/1151 (closed)
Too many icons show in navigation portlet https://github.com/plone/Products.CMFPlone/issues/1093 (Closed)
Add option to show thumbnails for images in Plone 5 content views https://github.com/plone/Products.CMFPlone/issues/1074 (closed)

Pull Requests

branchname: plip1734thumbs https://github.com/plone/buildout.coredev/tree/plip1734thumbs

enhancement regression review ready documentation cleanup feature (plip)

Source

fgrcon

Most helpful comment

@fgrcon actually it was just a small this in CHANGES, I fixed it with the Github interface :)

So we are done !!!
Congrats @fgrcon, your efforts are very appreciated!

ebrehault on 1 Jun 2017

🎉2 😄1 👍1

All 98 comments

http://jenkins.plone.org/job/pull-request-5.1/

fgrcon on 1 Sep 2016

http://jenkins.plone.org/job/pull-request-5.1/596/

fgrcon on 3 Sep 2016

The title of this PLIP is still WIP - shall the Framework Team vote on it? It needs a seconder.

jensens on 4 Oct 2016

@jensens @plone/framework-team : this is more or less finished. upgrade step (reapply profile,.. ) might be needed. anyone to second help ... most welcome !

to test / Try out : https://github.com/plone/buildout.coredev/tree/plip1734thumbs

http://jenkins.plone.org/job/pull-request-5.1/695 :

fgrcon on 7 Oct 2016

👍1

thanks for this!

@hvelarde @davilima6 or anybody else would second this plip?

May we rethink the mimetype icons?
I think we can do better than using an outdated set of pngs.

I found http://www.flaticon.com/packs/file-formats-icons and I am sure there are other, maybe better, icons out in the internet. Ideas?

jensens on 9 Oct 2016

👍1

I recently stumble upon this again while trying to make collective.cover compatible with Plone 5; this is becoming more and more complex and hard to understand and maintain.

configurability of this feature has to be done with CSS and not by a bunch of registry records; what was the point of using font icons if not?
lead image story is broken as mentioned here: https://community.plone.org/t/what-is-the-canonical-way-to-get-lead-image/2821?u=hvelarde

I'm not backing this until we have a clear migration path and documentation.

hvelarde on 10 Oct 2016

@hvelarde (cc @jensens, @plone/documentation-team ):
I agree, that you might have stumbled about something - but the question is what!

configurability of this feature has to be done with CSS and not by a bunch of registry records; what was the point of using font icons if not?

I only extended the options available in site control panel (as seems to be standard with plone...). Of course this settings are used to generate/modify css classes ....). Would you like to have your end users (e.g. site admins) to edit css ? - what a fantastic user experience!

lead image story is broken as mentioned here: https://community.plone.org/t/what-is-the-canonical-way-to-get-lead-image/2821?u=hvelarde

My proposed modifications don't even touch lead image implementation. As far as I can see, it also is not broken. As far as I understand the mentioned issue, your are unhappy with requirements/design of the lead image feature .
btw: I think, that a little bit (more?) of requirements engineering (including traceable documentation) could help plone development tremendously

I'm not backing this until we have a clear migration path and documentation.

migration path (?): I think one upgrade step (reapply profile for site settings ) should do , one more ( upgrade profiles for existing folders and collections in p.a.contenttypes to enable IThumbIconHandling ) might be useful (not implemented - need help).

Documentation is work in progress, see detailed information above as a preliminary sketch - but anyway, what is documentation for if nobody reads it?

For finishing/integrating documentation (much work to be done after modifications accepted I will need some advice/support from documentation team. (Btw: I located several spots in current documentation´concerning this topics which need corrections, clarifications, updates ... etc..)

fgrcon on 2 Nov 2016

@fgrcon I understand, but the point is we need to evaluate the icons story in Plone 5.1 before going further with this kind of new features, as they takes a lot of time and work, something you may already noticed.

IMO, the implementation of icons in Plone 5 (using fonts instead of plain images) is limited, more complicated for everyone, and has no real advantage over the previous one using images.

when I say we need a clear migration path and documentation, I meant for the icons story; so, I'm not against you feature, but in favor of reviewing some things before to make Plone 5.1 easier to maintain and configure.

hvelarde on 2 Nov 2016

@hvelarde , cc @plone/framework-team :
I am fully aware that the decision to use fonts instead of icon-images could be problematic (e.g.: how to generate an icon for an custom dexterity type ...) - but this was not my decision!. When I stumbled into all this more than one year ago i found quite a mess ! (icon fonts and icon images, often 404 ..., lot of confusion and dead code ...).

The bunch of prs is more or less the second step of cleanup, and tries to answer a number of issues reported. (I really spent a huge amount of time digging around sometimes undocumented or deprecated code. )

If it should be decided, to replace fontello by images again, it would not be much effort to adapt this.

Once again i opt for a better (transparent, traceable ,..., impact analysis) requirements engineering/design process!

Anyway I finish for now (open to serious discussion, tests...) rebased everything:
http://jenkins.plone.org/job/pull-request-5.1/895/

All pull requests:

If there is no interest in this, I will delete related branches and close all the prs (and just use the stuff on my own).

fgrcon on 2 Nov 2016

don't misunderstand me: if your work will make the maintenance easier, I'm totally fine with that. please don't trow it away.

as I have no time to review your changes in depth, and I don't know the implications, I can't second this, that's all.

hvelarde on 2 Nov 2016

In fact we plan to replace icons with svg's which are then inlined using a transform chain step. /cc @agitator

This is a massive plip. is it possible to divide it into 2-3 better consumable ones?

jensens on 2 Nov 2016

@jensens or anyone who has time to help ....:

Can anybody help me to understand the jenkins results above ? I could not spot any relevance for the mentioned prs or do I need to debug all the tests ?.

@jensens: the whole thing is not that massive or complex at all. Of course it would be possible to make some inconsistent patchwork out of it.

In fact we plan to replace icons with svg's which are then inlined using a transform chain step

sounds complicated - is there already any documentation / time frame ... for that? As long there is nothing working existing yet, I think the proposed changes would be better than nothing.

fgrcon on 2 Nov 2016

@fgrcon if you're fine just call me now, @agitator and I are working together at the moment and we would like to talk about this.

jensens on 2 Nov 2016

I second this proposal.

After a review and testing this in a site I really think this improves our handling of images in listings and portlets. It gives the control that was missing.

It also reintroduces file-types icons which many people are missing. The quality of this icons - located in MTR - are not the best, but @agitator and I are planning to replace them and other icons in another PR by inline-svgs, as alrady discussed with a broader audience and @RobZoneNet UX-Team at Plone conference in Boston (expect here a PLIP soon, but decoupled from this one)

@fgrcon I just found one glitch in the portlet: if there are file-type icons shown, the indent is wrong.
Also you're aware of the missing file type icons in folder_contents. I guess @thet can give you a hint where to hook in here.

jensens on 4 Nov 2016

👍1

@RobZoneNet would you please have a look at this PLIP from a UX point of view?

jensens on 4 Nov 2016

to test this PLIP simply checkout the plip1734thumbs branch in buildout.coredev. This is not standard procedure, but works fine.

jensens on 4 Nov 2016

@jensens: Thanks, great news !!!!
The glitches you mentioned, had been fixed already. But after rebasing last time I can not find structure/pattern.js anymore when I run in development mode(chrome web tools...). When I run plone-compile resources modifications in mockup/patterns/structure are not reflected in the results.
Dont know whats going wrong here ( @thet: any ideas ? )

fgrcon on 5 Nov 2016

update:
Resource registry > development mode develop js (plone-logged-in) works:
contents new

But: build bundle .... :

Bundle Builder
You are about to build the plone-logged-in pattern.
This may take some a bit of time so we will try to keep you updated on the progress.

Press the "Build it" button to proceed.

building javascripts
Saved javascript bundle, Build results:
FUNCTION
----------------
./++resource++mockup/inlinevalidation/pattern.js
./++resource++manage-portlets.js
.....
./++resource++plone-logged-in.js

building CSS bundle
1 css files to build
less compilation error on file http://192.168.1.15:8080/Plone1/++plone++static/plone-logged-in.less: Error: 'http://192.168.1.15:8080/Plone1/++resource++mockupless/patterns/tooltip/pattern.tooltip.less' wasn't found (404)
.......
less compilation error on file http://192.168.1.15:8080/Plone1/++plone++static/plone-logged-in.less: Error: 'http://192.168.1.15:8080/Plone1/++resource++mockupless/patterns/tooltip/pattern.tooltip.less' wasn't found (404)

I was not able to recompile resources in order to make it work in production mode. (plone-recompile ...'

confirm: identation of mimetype icons in (only navigation?-) portlets is wrong --> will try to fix it.

fgrcon on 6 Nov 2016

@jensens: tired of updating ....
http://jenkins.plone.org/job/pull-request-5.1/980/
some small css stuff still to fix (mostly unnecessary/ugly bullets in barceloneta ...)

btw.: huge troubles with compile resources, modifications only work in development mode (.Js only, css is totally broken ...) see also https://github.com/plone/Products.CMFPlone/issues/1843

fgrcon on 18 Nov 2016

Hi @fgrcon, your PLIP has been approved by the @plone/framework-team. If you have any question or if you need help during the implementation, do not hesitate to contact the team.

ebrehault on 22 Nov 2016

@plone/framework-team : thanks for approving. From my point of view implementation is mostly done (some smaller fixes might follow...). I will take care for documentation subsequently.
BUT: I need help with the testresults (as clear as mud to me !) and the issue mentioned in my last comment above
Btw: I will be more or less offline from 11/30 untill 12/15 (only email is possible).

fgrcon on 29 Nov 2016

one more try ...:
http://jenkins.plone.org/job/pull-request-5.1/1135

fgrcon on 4 Jan 2017

@fgrcon I ran your tests: http://jenkins.plone.org/job/pull-request-5.1/1311/

Regarding the CMFDiffTool failures, that's quite simple: in plone.app.contenttypes, you have added 5 new attributes to contents (via your IThumbIconHandling schema), so when we make a diff, we have 19 answers instead of 14.
I have created a new branch in CMFDiffTool where the tests are fixed accordingly: https://github.com/plone/Products.CMFDiffTool/tree/plip1734thumbs

ebrehault on 15 Feb 2017

I have also updated Products.CMFPlone with master. Let's see if the tests are bette now:
http://jenkins.plone.org/job/pull-request-5.1/1313/

ebrehault on 15 Feb 2017

@fgrcon As I was trying to understand the tests issues with plone.app.caching, I have noted few things:

I think we should not enable the IThumbIconHandling behaviour by default (most users will be happy with a side-wide setting)
unlike the site settings, local folder settings do not show a selection list but free text field:
with a default Barceloneta theme, there are bad margins in the portlet

ebrehault on 15 Feb 2017

@ebrehault: thanks a lot. Its fine for me not to enable the behavior by default. fixed in p.a.contenttypes
If i remember correctly the margin issue is only with navigation portlet ...

Right now I am very busy with diving :-). Unfortunately I will be back by end of next week and go on with work.

fgrcon on 19 Feb 2017

😄1

Diving!!! Lucky guy :)

ebrehault on 21 Feb 2017

@ebrehault @jensens ..:
I have fixed everything now (even got rid of the ugly and superfluous bullets in the portlets). Everything works fine for me but I suppose some of the tests (which are so fantastically documented ) will still not work.

PLEASE HELP !!

see http://jenkins.plone.org/job/pull-request-5.1/1380/ (fixed typo see below)

fgrcon on 6 Mar 2017

🎉2

@ebrehault : after not enabling behavior IThumbIconHandling by default we get the oposite errors
first we had 19 != 14 (http://jenkins.plone.org/job/pull-request-5.1/1311/)
including pull request https://github.com/plone/Products.CMFDiffTool/pull/24 we get 14 != 19 (http://jenkins.plone.org/job/pull-request-5.1/1380/)

If we ommit https://github.com/plone/Products.CMFDiffTool/pull/24 it looks fine again with CMFDiffTool see http://jenkins.plone.org/job/pull-request-5.1/1385/
This again makes me doubt about quality and reasonableness of many of the tests !

Remaining test failures:
plone.app.testing.layers.rst
plone.app.caching.tests.test_integration.TestOperations.test_disabled
plone.app.content.tests.test_widgets.BrowserTest.testUntranslatableMetadata
plone.app.portlets.tests.test_configuration.TestGenericSetup.testExport
plone.app.portlets.tests.test_navigation_portlet.TestRenderer.testBottomLevelZeroNoLimitRendering
plone.app.portlets.tests.test_review_portlet.TestPortlet.testInvokeAddview

fgrcon on 7 Mar 2017

after not enabling behavior IThumbIconHandling by default we get the oposite errors

yes it makes sense, now the behavior is not enabled, we go back to the initial situation, so just forget about my CMFDiffTool branch and use master

ebrehault on 7 Mar 2017

http://jenkins.plone.org/job/pull-request-5.1/1387/ (some tests fixed)
@jensens, @ebrehault : *I do not understand these 3 remaining test results - urgently need support.
I want to get this PLIP merged and closed possibly today !!!*

plone.app.testing.layers.rst

File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 183, in layers.rst
Failed example:
    getSiteManager()
Expected:
    <BaseGlobalComponents test-stack-2>
Got:
    <BaseGlobalComponents test-stack-3>

plone.app.caching.tests.test_integration.TestOperations.test_disabled

HTTP Error 500: Internal Server Error

  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/jenkins/workspace/pull-request-5.1@2/src/plone.app.caching/plone/app/caching/tests/test_integration.py", line 143, in test_disabled
    browser.open(self.portal['f1'].absolute_url())
  File "/home/jenkins/.buildout/eggs/zope.testbrowser-3.11.1-py2.7.egg/zope/testbrowser/browser.py", line 240, in open
    self.mech_browser.open(url, data)
  File "/home/jenkins/.buildout/eggs/mechanize-0.2.5-py2.7.egg/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/home/jenkins/.buildout/eggs/mechanize-0.2.5-py2.7.egg/mechanize/_mechanize.py", line 255, in _mech_open
    raise response

plone.app.content.tests.test_widgets.BrowserTest.testUntranslatableMetadata
u'Page' != u'Seite'

Page+ Seite

File "/home/jenkins/.buildout/eggs/unittest2-0.5.1-py2.7.egg/unittest2/case.py", line 340, in run
testMethod()
File "/home/jenkins/.buildout/eggs/mock-1.0.1-py2.7.egg/mock.py", line 1201, in patched
return func(*args, **keywargs)
File "/home/jenkins/workspace/pull-request-5.1@2/src/plone.app.content/plone/app/content/tests/test_widgets.py", line 561, in testUntranslatableMetadata
self.assertEqual(data['results'][0]['Type'], u'Seite')
File "/home/jenkins/.buildout/eggs/unittest2-0.5.1-py2.7.egg/unittest2/case.py", line 521, in assertEqual
assertion_func(first, second, msg=msg)
File "/home/jenkins/.buildout/eggs/unittest2-0.5.1-py2.7.egg/unittest2/case.py", line 923, in assertMultiLineEqual
self.fail(self._formatMessage(msg, standardMsg))
File "/home/jenkins/.buildout/eggs/unittest2-0.5.1-py2.7.egg/unittest2/case.py", line 415, in fail
raise self.failureException(msg)

fgrcon on 7 Mar 2017

@jensens: urgent please see https://github.com/plone/Products.CMFPlone/issues/1734#issuecomment-284687256 (updated)

fgrcon on 7 Mar 2017

hmm, possible is, this are test isolation problems. Otherwise, it does not make much sense. @ebrehault what do you think?

In Plone core we run tests in different isolation groups. Usually, its is not needed for PR's and PLIPs. But here it touches that many packages, it could be possible.

jensens on 7 Mar 2017

@fgrcon as far as I can see we do not have a PLIP job runner on Jenkins for https://github.com/plone/buildout.coredev/blob/5.1/plips/plip-1734-icons-and-thumbs.cfg - this may help.

@plone/testing-team @gforcada may you please create a Jenkins job for this PLIP configuration? TIA!

jensens on 7 Mar 2017

@fgrcon btw, the last commit at plip-1734-icons-and-thumbs.cfg is from @ebrehault and 20 days old. Please check if everything in there is set and up to date!

jensens on 7 Mar 2017

as far as can I see this should be ok. Its quite the same like in my approach (branching buildout.coredev ...).
I only updated existing prs - no new ones.

fgrcon on 7 Mar 2017

You do not need to branch, that's we have the plips folder for, with its jobs. Just run it with bin/buildout -c plips/plip-1734-icons-and-thumbs.cfg.

jensens on 7 Mar 2017

Btw., do tests are passing on your local machine if you run bin/alltests --all (or with robot tests bin/alltests --all)? (! takes a while !)

jensens on 7 Mar 2017

I know it takes some time, just started new local test again... .
last local test looked quite ok but I'm not sure anymore

fgrcon on 7 Mar 2017

tried to create jenkins job: https://github.com/plone/jenkins.plone.org/pull/199
@jensens: bin/buildout -c plips/plip-1734-icons-and-thumbs.cfg and my branch approach of course make no difference . last bin/test --all running localy.

Last rebases/merges done.

After that I will give up!!

fgrcon on 12 Mar 2017

Thanks @gforcada for http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/
@jensens : local tests give same results
how to proceed ? remaining failures seem to be caused by badly designed tests?
I definitely will not rebase eleven pull requests over the next weeks and months

fgrcon on 14 Mar 2017

finished merge with PLIP: Retina image scales #1483.

Local tests (bin/alltests -all):
``` Tear down zope.testrunner.layer.UnitTests in 0.000 seconds.

Finished tests for z3c.formwidget.query

Packages with test failures:
Failing tests in group plone_app_testing
Total time elapsed: 8501.199 seconds

Grand total: 90 packages, 1 failures

Failure in test Scenario Thememapper LESS builder (test_thememapper.robot)
Failure in test testUntranslatableMetadata (plone.app.content.tests.test_widgets.BrowserTest)
Failure in test /plone/bld51/plips/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst
Scenario: Thememapper LESS builder | FAIL |
Test Thememapper | FAIL |
Scenario: Location query :: This sometimes fails with: Element loc... | FAIL |
```
Robot Test Log.pdf

Jenkins
http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/3/

fgrcon on 26 Mar 2017

Testresults:

[X] Products.CMFPlone.tests.test_robot.RobotTestCase.Scenario Social settings are provided
Text Changes saved' did not appear in 30 seconds
??? manual tests work as designed
[X] Products.CMFPlone.tests.test_robot.RobotTestCase.Scenario Social tags are disabled
??? see above
[X] Products.PortalTransforms.tests.test_transforms.
SafeHtmlTransformsWithScriptTest.test_entities_outside_script unrelated to this plip - reproduced with vanilla 5.1 locally /does not occur with last release Products.PortalTransforms 2.2.2
http://jenkins.plone.org/job/plone-5.1-python-2.7 does not contain tests for Products.PortalTransforms?? cc: @tomgross
[X] Products.PortalTransforms.tests.test_xss.TestXSSFilter.test_22 unrelated to this plip - reproduced with vanilla 5.1 locally ....see above
[X] plone.app.testing.layers.rst
Failed doctest test for layers.rst File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 183, in layers.rst Failed example: getSiteManager() Expected: <BaseGlobalComponents test-stack-2> Got: <BaseGlobalComponents test-stack-3> ....
This works as isolated bin/test -s plone.app.testing
[x] plone.app.content.tests.test_widgets.BrowserTest.testUntranslatableMetadata
fixed: this was a bug (my bug).

u'Page' != u'Seite'
- Page+ Seite

  File "/home/jenkins/.buildout/eggs/unittest2-0.5.1-py2.7.egg/unittest2/case.py", line 340, in run
    testMethod()
  File "/home/jenkins/.buildout/eggs/mock-1.0.1-py2.7.egg/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/jenkins/workspace/plip-1734-icons-and-thumbs/src/plone.app.content/plone/app/content/tests/test_widgets.py", line 561, in testUntranslatableMetadata
    self.assertEqual(data['results'][0]['Type'], u'Seite')
  File "/home/jenkins/.buildout/eggs/unittest2-0.5.1-py2.7.egg/unittest2/case.py", line 521, in assertEqual
    assertion_func(first, second, msg=msg)
.....

additional robot test : TEST CASE: Scenario: Location query (non-critical)

@jensens, @ebrehault : How to proceed ? Please help!!!!

fgrcon on 27 Mar 2017

Wow, these test failures are scary. I observed the first two also a couple of times and the third and fourth should not happen too. They are for sure not related to this PR.
Number 5+6 could indicate a test isolation problem. Or something different!

Ping @plone/testing-team @gforcada @tisto @tomgross may one of you take a look on this? I am a bit lost with it.

jensens on 27 Mar 2017

@jensens without looking into it I would suspect that those test failures are a result of running the tests in a different order. Did you add a new package to the alltests group? Did you change the order? Did you introduce a new test fixture that might affect the test execution order (zope.testrunner tries to be smart and changes the test execution order to make the tests run faster).

I've been there multiple times. I'm pretty sure this has nothing to with the pr itself. Though, unfortunately, we still need to fix it. :(

tisto on 27 Mar 2017

@fgrcon any chance you recall adding a "transaction.commit()" to an integration test fixture test somewhere? Just (another) wild guess...

tisto on 27 Mar 2017

@fgrcon have you tried running the packages that have test failures individually? e.g. "bin/test -s plone.app.content". Do the failures still occur? If not, we have a test isolation problem...

tisto on 27 Mar 2017

@tisto .....: I never added something like transaction.commit() (I 'm just too silly for something like that. :-) )
and yes I run _bin/test -s something_ quite often:

e.g.: and just to draw attention to this again (see above):

Products.PortalTransforms.tests.test_transforms.
SafeHtmlTransformsWithScriptTest.test_entities_outside_script
unrelated to this plip - reproduced with vanilla 5.1 locally /does not occur with last release Products.PortalTransforms 2.2.2
http://jenkins.plone.org/job/plone-5.1-python-2.7 does not contain tests for Products.PortalTransforms??

fgrcon on 27 Mar 2017

@tisto, @jensens
updated see https://github.com/plone/Products.CMFPlone/issues/1734#issuecomment-289431784
new job started: http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/4/

fgrcon on 28 Mar 2017

Yeah, only one is left. And it looks like for some reason a component registry was pushed to the stack, but did not get popped on teardown. If have no idea why, and no idea where.

Failed doctest test for layers.rst
  File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 0

----------------------------------------------------------------------
File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 183, in layers.rst
Failed example:
    getSiteManager()
Expected:
    <BaseGlobalComponents test-stack-2>
Got:
    <BaseGlobalComponents test-stack-3>
----------------------------------------------------------------------
File "/home/jenkins/.buildout/eggs/plone.app.testing-5.0.6-py2.7.egg/plone/app/testing/layers.rst", line 263, in layers.rst
Failed example:
    getSiteManager()
Expected:
    <BaseGlobalComponents test-stack-2>
Got:
    <BaseGlobalComponents test-stack-3>

jensens on 28 Mar 2017

Though I don't understand everything what is going on behind the scenes, I consider the build green now. (the only one failure remaining is obviously an isolation issue).

@jensens: what next ???

fgrcon on 28 Mar 2017

Oops concurrent editing ...
if i run this one test locally, everything looks fine.

fgrcon on 28 Mar 2017

@fgrcon that's why is an isolation problem, there is another package that is running before the one showing the failure that somehow has a broken test setup, but you can only see it on your test rather than on the package with a broken test setup :-/

gforcada on 28 Mar 2017

@fgrcon @jensens I usually look at the full console output: http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/4/console where you can see at least two suspicious things:

a traceback from plone.protect (which does not cause an error though :-/
a AttributeError: 'NoneType' object has no attribute 'iteritems' a bit further down that seems related to ScalesVocabulary ... maybe that's more related??

gforcada on 28 Mar 2017

@gforcada @jensens
I could reproduce the problem with local bin/test -s plone.app.upgrade.
But even debugging for quite some time, I could not locate the bug.
Any help most appreciated.

fgrcon on 28 Mar 2017

green: http://jenkins.plone.org/job/plip-1734-icons-and-thumbs/6/ green

fgrcon on 31 Mar 2017

http://jenkins.plone.org/job/pull-request-5.1/1434/ green
http://jenkins.plone.org/job/pull-request-5.1/1440/ ~~will be also green :-)~~
oops plone.app.layout needed rebase :camel: :camel: :camel: ... : http://jenkins.plone.org/job/pull-request-5.1/1441/ -> green

PRs to merge :

fgrcon on 2 Apr 2017

@jensens

http://jenkins.plone.org/job/pull-request-5.1/1458/

one test with error left -but shlould work as designed!

bin/test -s plone.app.caching -t test_disabled:

Error in test test_disabled (plone.app.caching.tests.test_integration.TestOperations)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/plone/plip1734thumbs/src/plone.app.caching/plone/app/caching/tests/test_integration.py", line 144, in test_disabled
    browser.open(self.portal['f1'].absolute_url())
  File "/plone/_eggs/zope.testbrowser-3.11.1-py2.7.egg/zope/testbrowser/browser.py", line 240, in open
    self.mech_browser.open(url, data)
  File "/plone/_eggs/mechanize-0.2.5-py2.7.egg/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/plone/_eggs/mechanize-0.2.5-py2.7.egg/me

please review:
https://github.com/plone/documentation/blob/plip1734thumbs/develop/addons/upgrade_to_51.rst#content-type-icons

PRs to merge :

fgrcon on 7 Apr 2017

@fgrcon I just added this https://github.com/plone/plone.app.caching/commit/2d69fd5eb71030368f35f7df9224f2d2cbaf106d to p.a.caching master branch, would you mind running the PR job again?

With that rather than a "500 something happened" we would actually see a proper traceback.

gforcada on 7 Apr 2017

This seems to have solved it. Local test ok .
http://jenkins.plone.org/job/pull-request-5.1/1472/ -> green
thanks @gforcada !!

fgrcon on 7 Apr 2017

🎉1

I triggered a run of the plip job http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/7/

jensens on 7 Apr 2017

http://jenkins.plone.org/job/plip-1734-icons-and-thumbs/8/ (on node 5 ) green

@jensens: your run was on node 2 --- this never works correctly?
node 5, 6 are my favorites (hetzner is faster ...)

fgrcon on 7 Apr 2017

@fgrcon yes, I think node 2 produces sometimes odd results. I have no clue why.

@gforcada is there a way to not run tests on node2?

jensens on 7 Apr 2017

1(already disabled), 2, 3 and 4 are on rackspace (I think) and eventually should be removed, I might increase 5 and 6 with more slots and see if then they are not so slow so we can remove 1-4 completely...

gforcada on 7 Apr 2017

cc @jensens my daily rebases and merges (I feel like a :camel: sometimes ...)

http://jenkins.plone.org/job/pull-request-5.1/1475 --> green

fgrcon on 8 Apr 2017

I merged plone/plone.app.vocabularies#47 because it is just a bugfix and not directly PLIP related.

jensens on 9 Apr 2017

I merged plone/plone.app.layout#97 because this one only emphasise a deprecation we implicitly had all the time.

jensens on 10 Apr 2017

@jensens , @gforcada , @thet .....
Last call for ....
http://jenkins.plone.org/job/plip-1734-icons-and-thumbs/11/ green
http://jenkins.plone.org/job/pull-request-5.1/1549/ green

fgrcon on 15 Apr 2017

We have FWT meeting while PLOG, Tuesday evening. There we will decide if it is ready to merge. I try to finish my official review in the train to Naples.

jensens on 15 Apr 2017

@jensens: ping ping ???

fgrcon on 24 Apr 2017

I guess nobody is interested in that anymore - ther e are so many shiny things to come / much better but already in a mor e mature state ?

fgrcon on 26 Apr 2017

I would rather say that you had bad timing on landing the plip (hats off to you fir all the work) and the architectural problem of plone. If in top of that there's a conference, these lasts weks have been really busy.

I will make sure to bring this plip on the next framework team meeting.

Apologies for the delay!

gforcada on 26 Apr 2017

All related PR's have been closed as well. please reopen them also

rnixx on 27 Apr 2017

@rnixx I would do that when I'm back home nit with my phone 😃

Meanwhile you could reopen them as well if you wish.

gforcada on 27 Apr 2017

Ok one last try : http://jenkins.plone.org/job/plip-1734-icons-and-thumbs/1/
btw: what happened to jenkins (all old results gone) ?

fgrcon on 27 Apr 2017

needed to rebase mockup & CMFPlone
recompiled plone-logged-in bundle
http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/2/
http://jenkins.plone.org/job/pull-request-5.1/17

fgrcon on 27 Apr 2017

failing tests:
Products.CMFPlone.tests.testMigrationTool.TestMigrationTool.testDoUpgrades
plone.app.testing.layers.rst
plone.app.upgrade.tests.test_upgrade.TestUpgrade.testDoUpgrades

Error in test testDoUpgrades (Products.CMFPlone.tests.testMigrationTool.TestMigrationTool)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/jenkins/workspace/pull-request-5.1/src/Products.CMFPlone/Products/CMFPlone/tests/testMigrationTool.py", line 54, in testDoUpgrades
    self.setup.manage_doUpgrades(request=request)
  File "/home/jenkins/.buildout/eggs/Products.GenericSetup-1.8.7-py2.7.egg/Products/GenericSetup/tool.py", line 1053, in manage_doUpgrades
    step.doStep(self)
  File "/home/jenkins/.buildout/eggs/Products.GenericSetup-1.8.7-py2.7.egg/Products/GenericSetup/upgrade.py", line 166, in doStep
    self.handler(tool)
  File "/home/jenkins/workspace/pull-request-5.1/src/plone.app.upgrade/plone/app/upgrade/v41/alphas.py", line 95, in add_siteadmin_role
    portal.manage_permission(permission_id, ['Site Administrator', ], True)
  File "<string>", line 8, in manage_permission
  File "/home/jenkins/.buildout/eggs/AccessControl-3.0.13-py2.7-linux-x86_64.egg/AccessControl/requestmethod.py", line 70, in _curried
    return callable(*args, **kw)
  File "/home/jenkins/.buildout/eggs/Zope2-2.13.26-py2.7.egg/OFS/role.py", line 82, in manage_permission
    self, permission_to_manage, roles=roles, acquire=acquire)
  File "/home/jenkins/.buildout/eggs/AccessControl-3.0.13-py2.7-linux-x86_64.egg/AccessControl/rolemanager.py", line 212, in manage_permission
    escape(permission_to_manage))
ValueError: The permission <em>Change portal events</em> is invalid.

@gforcada @kakshay21
I suppose this failures are there because of https://github.com/plone/Products.CMFPlone/pull/2018 ?

fgrcon on 28 Apr 2017

@fgrcon probably you are right, though jenkins on 5.1 branches is fine...

As for the old jenkins jobs results, that was a mistake from me. The server had the disk almost full, so I started cleaning up, and I end up cleaning up too much 😨

gforcada on 28 Apr 2017

@gforcada: I could do with so efficient cleaning at home :-) but how to proceed with test failures?
Is not that urgent because @thet anyway will force me to rebase mockup once or twice a day (though the last prs look very very promising :-) ) ...

fgrcon on 28 Apr 2017

I think we need an official review and get the PLIP through the official FWT process as soon as possible. I already started a review but did not finish due to project load. Any help appreciated.

jensens on 28 Apr 2017

und täglich grüßt das murmeltier (Groundhog Day) ....
http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/3/ green
http://jenkins.plone.org/job/pull-request-5.1/28 green

fgrcon on 1 May 2017

@jensens I will give it a shot. As you have already started a review, is there anything special you would like me to review?

ebrehault on 2 May 2017

testing rebases ... : http://jenkins.plone.org/view/PLIPs/job/plip-1734-icons-and-thumbs/4/ OK
http://jenkins.plone.org/job/pull-request-5.1/116/

fgrcon on 31 May 2017

@fgrcon good, thanks! By the way, I am progressing in my review.

ebrehault on 31 May 2017

I have (finally) finished my review. The only small thing to fix seems to be this plone.api usage (see comments https://github.com/plone/plone.app.portlets/pull/85#discussion_r119466943 and https://github.com/plone/plone.portlet.collection/pull/15#discussion_r119469114 ) but for the rest, I am +1 for merging.

ebrehault on 1 Jun 2017

🎉2 👍1

@ebrehault: Thanks for reviewing
both comments will be fixed imediately (img tag - must have been in a state of mental derangement :-),
usage of api - I was not aware of this when started this work ...)

fgrcon on 1 Jun 2017

@ebrehault @jensens @gforcada ....
finished
jenkins jobs in queue: http://jenkins.plone.org/job/plip-1734-icons-and-thumbs/5/
http://jenkins.plone.org/job/pull-request-5.1/134/

fgrcon on 1 Jun 2017

🎉1

Tests are green :)
@jensens @gforcada shall we merge?

ebrehault on 1 Jun 2017

👍2

fgrcon opened this Issue on 30 Aug 2016

well deserved

:1st_place_medal:

rnixx on 1 Jun 2017

@fgrcon as this is for 5.1 only, I would say yes, let's merge now 👍

gforcada on 1 Jun 2017

Yes, ready for merge!

jensens on 1 Jun 2017

I guess https://github.com/plone/documentation/pull/809 should be merged, too ?

svx on 1 Jun 2017

@fgrcon there is a confict here https://github.com/plone/plone.app.content/pull/106, could you have a look?

ebrehault on 1 Jun 2017

@svx yes I did merge it

ebrehault on 1 Jun 2017

@ebrehault awesome ! thanks !!!!

svx on 1 Jun 2017

@fgrcon actually it was just a small this in CHANGES, I fixed it with the Github interface :)

So we are done !!!
Congrats @fgrcon, your efforts are very appreciated!

ebrehault on 1 Jun 2017

🎉2 😄1 👍1

Thanks !!

fgrcon on 1 Jun 2017

@fgrcon so what's your next PLIP going to be 😄

gforcada on 2 Jun 2017

Was this page helpful?

0 / 5 - 0 ratings

Related issues

Use Zope 5 in Plone 6

ale-rt · 3Comments

global_statusmessage viewlet is duplicated for many control panel pages in Plone 5.1

djowett-ftw · 5Comments

Content Type Icons no longer displayed in 5.2.2rc3

pbauer · 5Comments

Release checklist Plone 5.2.3

mauritsvanrees · 5Comments

Theming controlpanel changes make site unusable

erral · 3Comments

Products.cmfplone: PLIP: Cleanup and enhance icon and thumb aspects

Proposer : fgrcon

Seconder : Jens W. Klein

Abstract

Motivation

Assumptions

Proposal & Implementation

Deliverables

Risks

Participants

Related issues

Pull Requests

Most helpful comment

All 98 comments

Finished tests for z3c.formwidget.query

Grand total: 90 packages, 1 failures

Related issues