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

Using catalog_uri for pagination #87

Closed

Conversation

tdipisa
Copy link
Contributor

@tdipisa tdipisa commented Feb 20, 2017

Possible fix for #68 using the catalog_uri() method from utils.
Unit tests reviewed.

@tdipisa
Copy link
Contributor Author

tdipisa commented Feb 20, 2017

Hello @amercader. Please, can you take a look at this pull request?

@metaodi
Copy link
Member

metaodi commented Feb 21, 2017

I really like re-using the existing utils method. But it's important to note the difference between a URL and URI. The goal of the catalog_uri() method is to create a unique URI for the catalog, so if both the ckanext.dcat.base_uri and the ckan.site_url are not present it will either use the app uuid or simply generate a UUID to get a unique URI.

One could argue that in these cases the pagination is not important, but we would intentionally break it. I think it makes sense to build the pagination on top of ckan.site_url and use the current request URL as a fallback (just like I proposed in #69). I should probably finish my work on #69, so we finally have a working solution for issue #68.

@tdipisa
Copy link
Contributor Author

tdipisa commented Feb 22, 2017

Thank you @metaodi for your reply. We urgently need this fix to allow RDF harvesting. @amercader what do you think about that?

@amercader
Copy link
Member

@tdipisa I agree with @metaodi in that the site URL and the catalog URI may or may not be the same. Now that #69 is merged please check if that solves your issues with pagination.

@amercader amercader closed this Feb 24, 2017
@tdipisa
Copy link
Contributor Author

tdipisa commented Feb 24, 2017

Now it seems to work fine, thank you @amercader and @metaodi ;)

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.

None yet

3 participants