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

Use current package_type for urls #5189

Merged
merged 17 commits into from
Apr 1, 2020
Merged

Conversation

smotornyuk
Copy link
Member

Working with custom package types(especially, while using ckanext-scheming), I've noticed this quite a big problem in our templates/redirects. No matter, what kind of dataset you are creating, even though you are likely using URL like /CUSTOM-PACKAGE-TYPE/new, on progressing to the second step you'll be redirected to resource creation page on /DATASET/name/resource/new.We are just using dataset.edit, resource.new, etc. routes for URL generation, thus package_type is ignored.

Initially, i thought about hacking into url_for - this just requires few lines of code, but in this case, additional query into DB required and usage of package_type becomes implicit. Instead, even if it requires tons of changes, I'm leaning to direct updates of all url_for usages. Yep, it requires more work, but it:

  • explicit
  • if you expect that dataset.new leads to /dataset/new - you are still right
  • if you've provided some custom fix for this behavior inside your extension - it is still correct

@smotornyuk smotornyuk removed the WIP label Feb 15, 2020
@smotornyuk
Copy link
Member Author

I've updated everything single place I was managed to locate by now. As for tests - I'm not sure if just visiting all possible pages and checking all the URLs related to dataset/resource views would do the thing... any idea on this account?

@amercader amercader added the WIP label Feb 18, 2020
@MandanaMoshref
Copy link

Hi,

I updated my base.html accordingly, however, I receive this error:
GenerationException: url_for can only return a string, got unicode instead: agrihub_resource.read?id=parcel_llkkss8320000261

is this due to python version? or what?
How this can be solved for ckan 2.8?

Best regards
Mani

@smotornyuk
Copy link
Member Author

smotornyuk commented Feb 27, 2020

Hi, @MandanaMoshref .
This change won't work for CKAN v2.8 as it has quite a different implementation of controllers. Actually, there is only one package controller in v2.8, whereas in v2.9 it was replaced by two blueprints - dataset and resource.

Thus, this PR neither can be cherry-picked nor backported into v2.8 and the only thing you can do - manually re-define a bunch of templates and package routes.

Generally, before v2.9, any trivial changes to metadata schema were assuming that you are still working with Dataset - i.e. it should be safe to use all the same routes and templates(unless you are using IDatasetForm directly, not via ckanext-scheming). If you are going to create some entity, that behaves in a completely different way(ckanext-scheming) - you should define a custom controller with custom routes in addition to changes to metadata schema. This means, for such purpose you shouldn't rely only on ckanext-scheming.
In v2.9 these ideas were re-thinked and we decided that different package types should be more distinct - that's why I'm working on current PR. But this is a big change to the ideas of "what dataset is" and such changes cannot be simply applied to previous CKAN versions because it will only produce more bugs.

@smotornyuk smotornyuk removed the WIP label Feb 28, 2020
@wardi wardi self-assigned this Mar 3, 2020
@wardi
Copy link
Contributor

wardi commented Mar 31, 2020

Does this change let plugins override the views for specific dataset types?

@smotornyuk
Copy link
Member Author

Previously it didn't, but when you mentioned it, I've realized that change required for supporting extendible blueprints is quite trivial, so I've included it in the last commit. Here's the link, in case I'll add some style fixes before you'll have time to review changes: c832174

@wardi
Copy link
Contributor

wardi commented Apr 1, 2020

@smotornyuk this is great work. Good to merge?

@smotornyuk
Copy link
Member Author

It will be really great if this PR will be merged and available in v2.9(we have a few projects that are currently using custom CKAN fork because this change is essential for their workflow). So, if you have no objections and or any ideas as to some additional improvements that can be useful in conjunction with ckanext-scheming - let's merge it

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.

4 participants