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

Datapusher Core Extension #1200

Merged
merged 37 commits into from Oct 25, 2013
Merged

Datapusher Core Extension #1200

merged 37 commits into from Oct 25, 2013

Conversation

domoritz
Copy link
Contributor

Datapusher extension adds hooks to interact with the datapusher.

It also add a page to resource edit in order to resubmit a job.

@ghost ghost assigned domoritz Aug 19, 2013
@ghost ghost assigned kindly Aug 20, 2013
…tore. Instead of using the action or the template helper, check whether the plugin is enabled. Moved the action so that changing the url works even if the datapusher is disabled.
Conflicts:
	ckanext/datastore/tests/test_create.py
@domoritz
Copy link
Contributor Author

@kindly I removed the datapusher_enabled action and fixed one or two other small things. Ready for review.

@johnmartin
Copy link
Contributor

@kindly OK, I've finally got around to finishing the UI of this. I think I've caught all the conditions in a nice manner. Could you check for me and then if it's all OK, this is all good from my POV.

@ghost ghost assigned johnglover Oct 17, 2013
@@ -235,6 +235,8 @@ def make_map():
])))
m.connect('dataset_edit', '/dataset/edit/{id}', action='edit',
ckan_icon='edit')
m.connect('dataset_resources', '/dataset/resources/{id}',
action='resources', ckan_icon='time')
Copy link
Contributor

Choose a reason for hiding this comment

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

There is another 'dataset_resources' route defined on line 246 (with a different icon), should one of these be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Merge issue, fixed

'resource_id': res['id'],
'set_url_type': True
})
except KeyError:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this try/except. res['id'] is already accessed on line 95, so this will not be thrown by line 101. Is this to catch a KeyError in datapusher_submit? That doesn't seem great. Maybe this is meant to be ValidationError?

@johnglover
Copy link
Contributor

@kindly I've added one more thing for you to look at (the try/accept above), then I'm happy to merge when the tests are passing.

johnglover added a commit that referenced this pull request Oct 25, 2013
@johnglover johnglover merged commit e4fc2fa into master Oct 25, 2013
@johnglover johnglover deleted the 1200-datastore-read-only branch October 25, 2013 11:44
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

4 participants