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

Patches from hub.HealthData.gov #124

Closed
wants to merge 4 commits into from
Closed

Conversation

JoshData
Copy link
Contributor

Hi,

We're using CKAN behind hub.healthdata.gov (part of the new healthdata.gov).

We've had to make some patches to CKAN. Hoping to get these upstream so we don't have to worry about them when it's time to update.

FYI, the patch regarding accessibility was to address a legal requirement for U.S. federal government websites.

What do you think?

Best,

Josh

…on the package details age, instead of being listed as 'Author/Maintainer not given'
@tobes
Copy link
Contributor

tobes commented Sep 17, 2012

Thanks for these patches,

My initial thoughts

  1. Accessibility patches - these seem a good idea (along with any other accessibility stuff) - we should check all templates are updated including (new style templates)

  2. Escaping solr params - I know there was a patch related to escaping some things similar to this @icmurray was it you who did the patch(es) and did it fix this issue? Or was it unrelated?

  3. Email - this is for the email link so I'm not sure if this is the best place in the new templates we are using h.mail_to for the logic instead of the link field which seems better to me - personally I'd remove these fields from the database entirely. Adding the message into the database also breaks translations @kindly have you any thoughts on this. I'd be happy for this patch to be used and the fields deprecated (possibly forcibly in package_dictize) for new templates

@tobes
Copy link
Contributor

tobes commented Sep 18, 2012

I have made a fresh pull request including your accessibility patch #129 hopefully it will be included in the upcoming 1.8 release

the other two issues are still being considered

…tem default, which might be something unhelpful like latin1
@tobes
Copy link
Contributor

tobes commented Sep 24, 2012

@tauberer
Thanks for this. For us it is usually going to be easier to manage if you make a fresh branch/pull request for each feature you wish us to take as we can action them separately.

@kindly
This last commit (force utf8 not system encoding for postgres) feels like your domain

@JoshData
Copy link
Contributor Author

I'll split them up in the future, thanks!

tobes notifications@github.com wrote:

@tauberer
Thanks for this. For us it is usually going to be easier to manage if
you make a fresh branch/pull request for each feature you wish us to
take as we can action them separately.

@kindly
This last commit (force utf8 not system encoding for postgres) feels
like your domain


Reply to this email directly or view it on GitHub:
#124 (comment)

JT

Sent from my tablet. Please excuse my brevity.

@tobes
Copy link
Contributor

tobes commented Oct 10, 2012

accessibility stuff has been added to 1.8 release

@JoshData
Copy link
Contributor Author

Thanks!

@seanh
Copy link
Contributor

seanh commented Oct 12, 2012

The authors and maintainers names and emails fix has gone into CKAN 1.7.2, 1.8 and 2.0.

So I think there are still the legacy search params fix HHS@e383856 and the UTF8 encoding fix HHS@cbe6984 to merge

@amercader
Copy link
Member

I've created separated pull requests for the pending patches (#160, #161)
Closing this one.

@amercader amercader closed this Oct 19, 2012
JoshData added a commit to HHS/ckan that referenced this pull request Feb 4, 2013
I first submitted this in pull request ckan#124 50aad8d (against CKAN 1.7) which @amercader re-submitted in pull request ckan#129, which was applied to release-v1.8, but it was never applied to the new templates in master.

This is a requirement for U.S. federal government websites (or at least ours).
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