Add an utility function to get object paths (fixes #119) #120
Conversation
| self.obj = api.content.create(portal, type='News Item', id='foo') | ||
| api.content.transition(self.obj, 'publish') | ||
|
|
||
| def test_get_path_to_virtual_path(self): |
There was a problem hiding this comment.
this test is redundant with test_canonical_url in the test_behaviors.py module.
There was a problem hiding this comment.
Not exactly. These tests are not about canonical_urls . They're mean to test getting the paths based on the request, and how it changes with VHM.
| get_path_to_virtual_path(self.request, self.obj), | ||
| 'plone/foo') | ||
|
|
||
| def test_get_path_to_virtual_path_using_virtualhostmonster(self): |
There was a problem hiding this comment.
this test must be moved to the test_behaviors.py module; rename it to test_canonical_url_with_virtual_host_root.
| @@ -128,3 +128,11 @@ def get_valid_objects(brains): | |||
| logger.warn(msg.format(b.getPath())) | |||
| continue | |||
| yield obj | |||
|
|
|||
|
|
|||
| def get_path_to_virtual_path(request, item): | |||
There was a problem hiding this comment.
I think there's no need to create this function; just place the code inside the assign_canonical_url subscriber, as this is the only place where it's called..
There was a problem hiding this comment.
And how I suppose to unittest it? 😉
There was a problem hiding this comment.
you should test just the final result as in the test I uncommented in the behavior.
| # FIXME: we're currently ignoring the Plone site id | ||
| # https://github.com/collective/sc.social.like/issues/119 | ||
| path = '/'.join(obj.getPhysicalPath()[2:]) | ||
| path = get_path_to_virtual_path(self.request, obj) |
There was a problem hiding this comment.
we need to fix this on a different way: this form is only used to assign a canonical URL to objects created before the package was installed/upgraded.
now imagine the following case: Plone site with the id visible to the end user migrating to a site where the id is no longer available, that is from http://www.example.org/Plone to http://www.example.org/.
we're currently not covering this case and the only solution I can think about is to add a Bool field to enforce the use of the site id or a ASCIILine field that lets you add an arbitrary string.
we need to think about this.
There was a problem hiding this comment.
Yeah, that's tough. I can't see a safer way to get the previous URL than accessing it with it (eg. with VHM).
However, asking the user to choose to use or not the Site Id as you said, sounds better than ask for the old VirtualHostMonster URL. 🤔
Is this case, we should allow path in the canonical_url. Imagine http://localhost:8080/VirtualHostBase/http/foo.com:80/Plone/VirtualHostRoot/_vh_intranet
Just saying to not use the Site Id isn't enough, you have to say that the root URL was http://foo.com/intranet/
| # FIXME: we're currently ignoring the Plone site id | ||
| # https://github.com/collective/sc.social.like/issues/119 | ||
| path = '/'.join(obj.getPhysicalPath()[2:]) | ||
| path = get_path_to_virtual_path(event.request, obj) |
There was a problem hiding this comment.
just do the conversion right here; you'll need to get the request on a different way (probably using getRequest() from zope.globalrequest) as the AfterTransitionEvent doesn't include it.
There was a problem hiding this comment.
While reviewing #118, I had a bad time trying to reach the code in the subscriber. Is it really working?
There was a problem hiding this comment.
yes it is, why you had such bad time? can you elaborate?
|
@idgserpro I think we need another pair of eyes here; can you help? |
|
Does anybody have something to add about it? @hvelarde @idgserpro |
Closes #119