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

Conditional DataStore Tab in Resource Edit #190

Merged

Conversation

JVickery-TBS
Copy link
Contributor

feat(templates): conditional DataStore tab;

  • Added helper for valid xloader formats.
  • Added condition to display the DataStore tab in Resource Edit.

- Added helper for valid xloader formats.
- Added condition to display the DataStore tab in Resource Edit.
@ThrawnCA
Copy link
Collaborator

Looks good to me, but I'm not a maintainer here. Do you want to open a pull request to the qld-gov-au fork, ideally the develop branch?

Actually, while you're working on this, do you think you could add a datastore tab link to the resource view page when appropriately authorised? Would be nice to be able to go there in one click.

…tore link in resource read;

- Added condition for `datastore_rw_resource_url_types`.
- Added DataStore action link to resource read if perms are correct.
…, added new helper;

- Added the conditional DataStore action link to more templates.
- Added another new helper to handle possible non-existant core helper.
@JVickery-TBS
Copy link
Contributor Author

DataStore action link now added to more templates as to feedback here: ckan/ckan#7647 (review)

Copy link
Collaborator

@ThrawnCA ThrawnCA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments on qld-gov-au#49

@ThrawnCA
Copy link
Collaborator

Don't get me wrong, this is a promising feature, I just want to help ensure it's at its best.

- Combined helpers and conditions into one helper.
- Moved `XLoaderFormats` class to utils script to prevent circular imports.
@JVickery-TBS
Copy link
Contributor Author

Comments addressed from qld-gov-au#49

@JVickery-TBS
Copy link
Contributor Author

@ThrawnCA this PR could just get marked for a release alongside the next CKAN release.

As the templates are made for the latest ckan, users can just override them if they need to for older releases.

@ThrawnCA
Copy link
Collaborator

@ThrawnCA this PR could just get marked for a release alongside the next CKAN release.

You could, but then no one can do any testing of it now. I intend to deploy this on data.qld.gov.au, but we're still on 2.9, currently working on the 2.10 upgrade. If even 2.10 can't use it... it'll be potentially a long time before we can give any feedback.

@wardi
Copy link
Contributor

wardi commented Jun 20, 2023

In extensions it's desirable to be able to support a number of ckan releases. The template blocks won't be visible in current releases but do they prevent people from using the feature?

@ThrawnCA
Copy link
Collaborator

The template blocks won't be visible in current releases but do they prevent people from using the feature?

We've deployed it on dev.data.qld.gov.au and it does nothing, no change. It doesn't have any errors, the new blocks are just ignored.

@JVickery-TBS
Copy link
Contributor Author

These pieces of templates were not in Xloader at all. These templates and blocks are completely new in this PR. So it does not affect the functionality of the extension at all.

The only template the existed before was package/resource_edit_base.html and the only change to that in this PR is the new conditional tab.

So all of the newly added templates could be considered "for the future"

@ThrawnCA
Copy link
Collaborator

ThrawnCA commented Aug 1, 2023

Why not just add the new link to the resource_actions_inner block instead? That one exists in both old and new CKAN versions. Or action_manage, which exists in current released 2.10.

ckanext/xloader/helpers.py Outdated Show resolved Hide resolved
# Conflicts:
#	ckanext/xloader/plugin.py
### RESOLVED.
- Added old template blocks for version 2.10 and less.
- Added falsy check on url_type in template helper method.
ckanext/xloader/helpers.py Outdated Show resolved Hide resolved
ckanext/xloader/utils.py Show resolved Hide resolved
- Added in line comments.
- Used shorthand syntax.
@ThrawnCA ThrawnCA merged commit 3ae7ae3 into ckan:master Feb 1, 2024
3 checks passed
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