+
Skip to content

Upgrade to CKAN 2.11 #2

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

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Upgrade to CKAN 2.11 #2

wants to merge 15 commits into from

Conversation

ntwalibas
Copy link
Collaborator

Description

This PR performs a major refactoring and enhancement:

  • It refactors the old structure of the extension so it follows the structure of our existing extensions,
  • It adds a functionality to track the number of page views.

It remains to have the following properly implemented:

  • Make sure to delete package and resource download data when the corresponding dataset/resource are deleted.

Testing

Tests to follow while functionality is being reviewed.

Documentation

The documentation needs to be improved so it is easier to install this extension in new projects.

Checklist

  • The Jira ticket for this issue has been updated to "Ready to Review" or equivalent.
  • I have developed these changes in discussion with the appropriate project manager.
  • My code follows the general Fjelltopp documentation (see Confluence).
  • I have made corresponding changes to the Fjelltopp documentation (see Confluence).
  • I have rebased this branch with master.
  • New dependency changes have been committed.
  • I have added automated tests that prove my fix is effective or that my feature works.
  • New and existing tests pass locally with my changes.
  • My changes generate no new warnings.
  • I have performed a self-review of my own code.
  • I have assigned at least one reviewer.
  • I have assigned at least one label to this PR: "patch", "minor", "major".

@ntwalibas ntwalibas marked this pull request as draft November 28, 2024 10:23
@ChasNelson1990
Copy link
Member

@ntwalibas - should we add a release tag to the previous version so that projects can continue to easily use the version from before the 2.11 migration

Copy link
Member

@jonathansberry jonathansberry left a comment

Choose a reason for hiding this comment

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

Wow this is a big change though! Hope it's all been properly tested. A couple of small comments from perusing through, but it's such a large PR it's difficult to check coherence in any meaningful way by review.

import ckan.views.resource as resource
import ckan.plugins.toolkit as tk
import ckan.views.api as api
from ckan.common import g
Copy link
Member

Choose a reason for hiding this comment

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

Can get this from the toolkit: toolkit.g

Comment on lines +50 to +51
context = {"model": model, "session": model.Session, "user": user}
package = toolkit.get_action("package_show")(context, {"id": path_id})
Copy link
Member

Choose a reason for hiding this comment

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

It is my understanding that toolkit.get_action automatically adds the model and session and user to the context. I believe this also means you don't need to import the model.

Suggested change
context = {"model": model, "session": model.Session, "user": user}
package = toolkit.get_action("package_show")(context, {"id": path_id})
package = toolkit.get_action("package_show")({}, {"id": path_id})

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.

3 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载