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 the full organization dict to display orgs #1605

Merged
merged 3 commits into from
May 19, 2014
Merged

Conversation

nigelbabu
Copy link
Contributor

The package page uses only the bits of the organization dict that is returned
from package_show, this doesn't have the image_display_url and therefore
organization images are not shown anymore. This uses a new helper to get the
full organization dict. Fixes #1592.

The package page uses only the bits of the organization dict that is returned
from package_show, this doesn't have the image_display_url and therefore
organization images are not shown anymore. This uses a new helper to get the
full organization dict.
@nigelbabu nigelbabu mentioned this pull request May 5, 2014
@joetsoi
Copy link
Contributor

joetsoi commented May 5, 2014

I don't like the pokemon gotta catch 'em all exception handler. I think this is a good case as to why there should be a ckan.exceptions module, it'd make writing stuff like this much easier. For now I'd be happy if it just caught the NotFound, ValidationError and NotAuthorized

@nigelbabu
Copy link
Contributor Author

In a template helper, we need to catch everything or it throws a traceback that we can't control. Swallowing the exception is the right move here.

@joetsoi
Copy link
Contributor

joetsoi commented May 6, 2014

I get where you are coming from, we just end up with 500s with uncaught exceptions atm, but getting all the template helpers to generically catch all exception sounds really wrong to me. Ideally I would like something along the lines of:

Any non caught exception to get handled higher up in the chain when the template is rendered. We should have a try catch block around there so that when in debug mode you reraise for the full traceback, and in production you could set it to log the unhandled exception and try to handle it gracefully.

Although you have finer grained control of how to handle it gracefully if handled down here in the helper functions.

@nigelbabu does that sound sane?

@nigelbabu
Copy link
Contributor Author

That sounds fairly sane, yes.

@joetsoi joetsoi merged commit 2a33125 into master May 19, 2014
@joetsoi joetsoi deleted the 1592-pkg-org-img branch May 19, 2014 12:00
@joetsoi
Copy link
Contributor

joetsoi commented May 19, 2014

summary from dev meeting: people prefer things exploding rather than attempting to handle it gracefully as that can just make things even harder to debug.

Still I wouldn't mind a better exception hierarchy as we've discussed before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Organization image not shown when viewing dataset
3 participants