Re: [Framework-Team] PLIP #126 ready for review
snip * Running the tests of plone.app.controlpanel on a vanilla checkout of the plip buildout generated one failure for me (Ubuntu/packaged uncustomized Python 2.4): ... Set up Products.PloneTestCase.layer.PloneSite in 18.665 seconds. Running: . Failure in test /home/ritz/buildouts/reviewing/plip126-link-redirects/src/plone.app.controlpanel/plone/app/controlpanel/tests/types.txt Failed doctest test for types.txt File /home/ritz/buildouts/reviewing/plip126-link-redirects/src/plone.app.controlpanel/plone/app/controlpanel/tests/types.txt, line 0 -- File /home/ritz/buildouts/reviewing/plip126-link-redirects/src/plone.app.controlpanel/plone/app/controlpanel/tests/types.txt, line 75, in types.txt Failed example: self.browser.contents Expected: '...Globally addable... ...Allow comments... ...Visible in searches... ...input id=redirect_links... ...type=checkbox... ...name=redirect_links:boolean... ...checked=checked... ...label for=redirect_links... ...Redirect links to remote url...' Got: '\n \n\n\n!DOCTYPE html PUBLIC\n -//W3C//DTD XHTML 1.0 Transitional//EN\n http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd;\n\n\nhtml xmlns=http://www.w3.org/1999/xhtml; xml:lang=en\n lang=en\n\n (truncated by me here) Guess it's because it expects ...Redirect links to remote url...' but it gets ...Redirect immediately to link target... Just for the record, this was broken for a brief time on our branch due to some jostling around of language, but was resolved in the following changeset: http://dev.plone.org/plone/changeset/24272 I hope that lies w/in your as of 2009-01-16 range (e.g. it was fixed right after you started your review). Not sure though. I'm not seeing a failure right now at least. I can look at the CMFPlone failures now. Andrew ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #126 ready for review
David Glick wrote: PLIP 126 (Link type should automatically redirect when accessed directly) has been implemented. You can get the review buildout from: http://svn.plone.org/svn/plone/review/plip126-link-redirects I'm going to paste the contents of http://svn.plone.org/svn/plone/review/plip126-link-redirects/PLIP_126_README.txt here for ease of any discussion... Review bundle for PLIP 126: Link type should automatically redirect when accessed directly == As Danny triggered some discussion already I'll add my comments (which I don't consider finished yet - that's why I didn't post them up to now). It's been a while that I did this and I might not be up-to-date with everything. Raphael's comments == (as of 2009-01-16) * on a new site the link type and the configuration options work as advertised * the migration worked for me on a randomly picked site of mine. * I don't understand why 'link_view' isn't made available as an alternative view on instance base still. That could easily be achieved by leaving it in the Available view methods property of the FTI. That way one could control this behavior on instance base even. At the very least this should be properly documented. * running the tests for CMFPlone I get 3 failures but they are all unrelated to the changes in question here (UnicodeSplitter, Latin1 processing, and migration related stuff) * Running the tests of plone.app.controlpanel on a vanilla checkout of the plip buildout generated one failure for me (Ubuntu/packaged uncustomized Python 2.4): ... Set up Products.PloneTestCase.layer.PloneSite in 18.665 seconds. Running: . Failure in test /home/ritz/buildouts/reviewing/plip126-link-redirects/src/plone.app.controlpanel/plone/app/controlpanel/tests/types.txt Failed doctest test for types.txt File /home/ritz/buildouts/reviewing/plip126-link-redirects/src/plone.app.controlpanel/plone/app/controlpanel/tests/types.txt, line 0 -- File /home/ritz/buildouts/reviewing/plip126-link-redirects/src/plone.app.controlpanel/plone/app/controlpanel/tests/types.txt, line 75, in types.txt Failed example: self.browser.contents Expected: '...Globally addable... ...Allow comments... ...Visible in searches... ...input id=redirect_links... ...type=checkbox... ...name=redirect_links:boolean... ...checked=checked... ...label for=redirect_links... ...Redirect links to remote url...' Got: '\n \n\n\n!DOCTYPE html PUBLIC\n -//W3C//DTD XHTML 1.0 Transitional//EN\n http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd;\n\n\nhtml xmlns=http://www.w3.org/1999/xhtml; xml:lang=en\n lang=en\n\n (truncated by me here) Guess it's because it expects ...Redirect links to remote url...' but it gets ...Redirect immediately to link target... Potentially more to follow Raphael ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #126 ready for review
Ok, I reviewed this plip and these are my findings: PLIP 126 framework review #3 by Danny Bloemendaal, 2009-01-27 = review steps the bundle was reviewed on OSX 10.5.6 doing the following: * bundle checkout, buildout etc * manual function tests to verify things work as intended notes and observations -- I reviewed this plip from a UI perspective of course. So I started by creating a folder as admin with a Link in there. Published both items. In another browser I visited as an anonymous user and opened the folder. Both the navtree and the link redirected me to the target url. As admin I also visited the folder and the navtree links me to the Link object while the folder listing redirects me to the target url. I checked what the default setting is for this redirecting and Redirect immediately to link target was not checked. Then how is it possible that it does redirect for anonymous and for admin in the folder listing? In my opinion, if you offer this option then it should not redirect when unchecked. Checking the option doesn't seem to change anything for anonymous and logged-in users without edit permissions. It keeps redirecting. The only change I notice when checking this option is the portal-like info message in the landing page for the Link object. So it looks as if this feature is not working at all or I am missing the entire point (which is also bad because I'm doing what I actually do best: being a dumb user) Seeing this and at least thinking about the intentions I begin to wonder why this isn't implemented as the comments option. There you have a site setting for the type if you want to allow commenting or not. If allowed you can still overrule this for each instance (you can either user the site's default or control locally). I also am not in favor of the Info message. I'd leave it out. If the site admin or the site designer decided to do automatic linking, then you don't need to show this decision in every view page. I'd put in the edit form instead where it belongs. It's a setting after all. Conclusion -- It seems to not work so for now (until someone explains me what I'm doing wrong and if so why I misinterpreted the entire functionality): -1 On 26 jan 2009, at 00:52, Andreas Zeidler wrote: On Jan 13, 2009, at 1:37 AM, David Glick wrote: PLIP 126 (Link type should automatically redirect when accessed directly) has been implemented. i've added my review notes in http://dev.plone.org/plone/changeset/24687 in short: very nice, +1 for merging. thanks david and andrew! cheers, andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.1.7 released! -- http://plone.org/products/plone/ PGP.sigATT1.txt ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #126 ready for review
On 27 jan 2009, at 20:19, Danny Bloemendaal wrote: As admin I also visited the folder and the navtree links me to the Link object while the folder listing redirects me to the target url. An additional remark. I notice the consideration in the readme for not adapting the various views that have conditions like folder_listing. I don't agree with the choice not to change them. If you give the option to have this redirecting or not, I'd say you *have* go all the way. Right now it is confusing because it only works partially. That doesn't feel right at all. You either do it right or leave it as it is now in my opinion. So, for this choice alone i tend to give it a -1. On 26 jan 2009, at 00:52, Andreas Zeidler wrote: On Jan 13, 2009, at 1:37 AM, David Glick wrote: PLIP 126 (Link type should automatically redirect when accessed directly) has been implemented. i've added my review notes in http://dev.plone.org/plone/changeset/24687 in short: very nice, +1 for merging. thanks david and andrew! cheers, andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.1.7 released! -- http://plone.org/products/plone/ PGP.sigATT1.txt ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #126 ready for review
On Jan 27, 2009, at 11:19 AM, Danny Bloemendaal wrote: notes and observations -- I reviewed this plip from a UI perspective of course. So I started by creating a folder as admin with a Link in there. Published both items. In another browser I visited as an anonymous user and opened the folder. Both the navtree and the link redirected me to the target url. As admin I also visited the folder and the navtree links me to the Link object while the folder listing redirects me to the target url. I checked what the default setting is for this redirecting and Redirect immediately to link target was not checked. Then how is it possible that it does redirect for anonymous and for admin in the folder listing? In my opinion, if you offer this option then it should not redirect when unchecked. Checking the option doesn't seem to change anything for anonymous and logged-in users without edit permissions. It keeps redirecting. The only change I notice when checking this option is the portal-like info message in the landing page for the Link object. So it looks as if this feature is not working at all or I am missing the entire point (which is also bad because I'm doing what I actually do best: being a dumb user) This behavior is due to the decision that I made and noted in the PLIP readme: * I experimented with, but ultimately deferred removing the conditional code in many listing views that handles Links as a special case in order to link to the target URL. This becomes unnecessary if redirect_links is true, because you can go to the Link like any other content type and its default view will take care of redirecting to the target. However, setting redirect_links to True seemed like too big of a behavior change for the 3.x series, so I have instead added a draft PLIP suggesting this change for 4.x: http://dev.plone.org/plone/ticket/8867 So, as the PLIP is currently implemented, the Redirect immediately to link target setting does not affect the behavior when Links appear in navigation. Danny, you're right that this is confusing and it's bad that the control panel setting doesn't seem to have an effect. The desired behavior we'd like to have is that links redirect, or don't redirect, based on the setting in the control panel, regardless of how someone accessed the link. The tricky question is what to do with existing sites that are expecting certain behavior in the 3.x series. If we make everything be controlled by the Redirect immediately to link target setting, then we cannot set it to True without changing the behavior when links are accessed via a direct URL. But we cannot set it to False without changing the behavior when links appear in navigation. However, at the moment it seems to me that the former would have been a better option, since Links are more often accessed via navigation than via a direct URL. So I probably made a bad decision when implementing this PLIP. At this point I would like to change the PLIP implementation to: 1. re-add the changes I made to listings so that Links aren't handled specially, and the redirect behavior is handled by the link view rather than by the navigation generating logic 2. change the default value of the Redirect immediately to link target setting to True, so that the behavior as viewed by the end user of clicking on a link in navigation remains the same 3. add documentation of how to restore the existing pre-3.3 behavior (links in navigation redirect, but Links when accessed via a direct URL do not) by changing the default view for Links back to link_view instead of the new link_redirect_view which contains the logic that checks the configlet setting. But I'm not sure whether the PLIP process allows me to make changes at this point... Seeing this and at least thinking about the intentions I begin to wonder why this isn't implemented as the comments option. There you have a site setting for the type if you want to allow commenting or not. If allowed you can still overrule this for each instance (you can either user the site's default or control locally). I didn't do it this way because it seemed like more overhead than it was worth to have this setting for each content type and for each instance. The use case of needing to have some links redirect and some not seemed advanced enough that requiring some simple customizations of the view to make it happen didn't seem like a big problem. Anyway, this seems like the sort of question that should have been raised when the PLIP concept was approved; it's not really a problem with the implementation of the proposed change per se. I also am not in favor of the Info message. I'd leave it out. If the site admin or the site designer decided to do automatic linking, then you don't need to show this decision in every view page. I'd put in the edit form instead
Re: [Framework-Team] PLIP #126 ready for review
On 27 jan 2009, at 21:07, David Glick wrote: On Jan 27, 2009, at 11:19 AM, Danny Bloemendaal wrote: notes and observations -- I reviewed this plip from a UI perspective of course. So I started by creating a folder as admin with a Link in there. Published both items. In another browser I visited as an anonymous user and opened the folder. Both the navtree and the link redirected me to the target url. As admin I also visited the folder and the navtree links me to the Link object while the folder listing redirects me to the target url. I checked what the default setting is for this redirecting and Redirect immediately to link target was not checked. Then how is it possible that it does redirect for anonymous and for admin in the folder listing? In my opinion, if you offer this option then it should not redirect when unchecked. Checking the option doesn't seem to change anything for anonymous and logged-in users without edit permissions. It keeps redirecting. The only change I notice when checking this option is the portal-like info message in the landing page for the Link object. So it looks as if this feature is not working at all or I am missing the entire point (which is also bad because I'm doing what I actually do best: being a dumb user) This behavior is due to the decision that I made and noted in the PLIP readme: * I experimented with, but ultimately deferred removing the conditional code in many listing views that handles Links as a special case in order to link to the target URL. This becomes unnecessary if redirect_links is true, because you can go to the Link like any other content type and its default view will take care of redirecting to the target. However, setting redirect_links to True seemed like too big of a behavior change for the 3.x series, so I have instead added a draft PLIP suggesting this change for 4.x: http://dev.plone.org/plone/ticket/8867 So, as the PLIP is currently implemented, the Redirect immediately to link target setting does not affect the behavior when Links appear in navigation. Danny, you're right that this is confusing and it's bad that the control panel setting doesn't seem to have an effect. Well, it was all about expectation (and probably not having read the readme correctly although having read the plip twice and I didn't see that it was just for redirecting when you access the url directly and not through the various navigation tools. The desired behavior we'd like to have is that links redirect, or don't redirect, based on the setting in the control panel, regardless of how someone accessed the link. Indeed. You need to make the behaviour predictable for the editors/ administrators. The tricky question is what to do with existing sites that are expecting certain behavior in the 3.x series. That behaviour was 'broken' to begin with. But ok... If we make everything be controlled by the Redirect immediately to link target setting, then we cannot set it to True without changing the behavior when links are accessed via a direct URL. But we cannot set it to False without changing the behavior when links appear in navigation. However, at the moment it seems to me that the former would have been a better option, since Links are more often accessed via navigation than via a direct URL. So I probably made a bad decision when implementing this PLIP. I'd say that when the switch is off, it behaves as 3.3. So, redirect=off, no redirect occures at all. When redirect=on everything redirects: direct urls, navigation elements, lists etc. At least that makes it predictable. At this point I would like to change the PLIP implementation to: 1. re-add the changes I made to listings so that Links aren't handled specially, and the redirect behavior is handled by the link view rather than by the navigation generating logic This makes it way much easier to customize the entire Link behaviour. 2. change the default value of the Redirect immediately to link target setting to True, so that the behavior as viewed by the end user of clicking on a link in navigation remains the same 3. add documentation of how to restore the existing pre-3.3 behavior (links in navigation redirect, but Links when accessed via a direct URL do not) by changing the default view for Links back to link_view instead of the new link_redirect_view which contains the logic that checks the configlet setting. Sounds ok with me. But I'm not sure whether the PLIP process allows me to make changes at this point... You have my vote. /me looks at the others Seeing this and at least thinking about the intentions I begin to wonder why this isn't implemented as the comments option. There you have a site setting for the type if you want to allow commenting or not. If allowed you can still overrule this for each instance (you can either user the site's default or control locally). I didn't do it this way because it seemed
