CKAN: Ticket #2445: Create demo dataset add related page

milestone set

aron.carroll — Mon, 28 May 2012 13:40:27 GMT

milestone set to current-ckan-sprint-2012-05-29

description changed

aron.carroll — Mon, 28 May 2012 13:40:56 GMT

description modified (diff)

milestone changed

aron.carroll — Mon, 28 May 2012 14:11:53 GMT

milestone changed from ckan-sprint-2012-05-29 to current-ckan-sprint-2012-06-25

cc, description changed

aron.carroll — Wed, 30 May 2012 10:56:17 GMT

cc ross added
description modified (diff)

owner, priority, status changed

ross — Wed, 30 May 2012 10:59:37 GMT

owner changed from aron.carroll to ross
priority changed from major to blocker
status changed from new to accepted

Yup. Will look at this Thurs when I'm back in.

toby — Wed, 30 May 2012 11:42:41 GMT

@Ross you will need to do this based off the feature-2375-demo-theme branch - you can use that branch or one split from it.

new templates want to be in templates/... the old templates live in templates_legacy if you need to copy them across that's fine

talk to aron or I if you need to know anything

ross — Thu, 31 May 2012 13:47:30 GMT

Okay, the 303 on validation failure seems a little excessive though given that we're posting to that URL. Is a 200 okay?

ross — Thu, 31 May 2012 13:49:21 GMT

Am working in feature-2445-add-related-new

toby — Thu, 31 May 2012 13:57:04 GMT

@ross

the validation error seems correct to use 200

for the created the 303 seems ok but i'd just use pylons redirect myself

Also did aron say we want this as a javascript free add item form

ross — Thu, 31 May 2012 14:02:46 GMT

He didn't say but I assumed yes given current impl uses dodgy JS->API.

aron.carroll — Thu, 31 May 2012 14:06:01 GMT

I generally take the approach that a form submitted via POST should always redirect to a page. This prevents accidental user resubmissions when hitting refresh as well as the annoying popup saying "are you sure you want to re-submit".

http://en.wikipedia.org/wiki/Post/Redirect/Get

Looking at the rest of CKAN it doesn't do this so I'll leave it up to you.

ross — Thu, 31 May 2012 14:08:29 GMT

redirects are fine, of course, but redirect on validation failure seems painful ?

Also templates in templates_legacy?

toby — Thu, 31 May 2012 14:28:10 GMT

@aron I'm fine with the redirect thinking it's just the specific 303 rather than just whatever pylons does.

@ross, templates_legacy are there for ckan users who have their own templates as we are completely rewriting all the existing templates

there is a lovely .ini setting to switch between the new and legacy modes

owner, status changed

ross — Fri, 01 Jun 2012 09:19:00 GMT

owner changed from ross to aron.carroll
status changed from accepted to assigned

I *think* this is done in feature-2445-add-related-new although I may have missed something, if you could take a look and let me know if it is what you wanted.

owner, priority changed

aron.carroll — Tue, 05 Jun 2012 15:45:21 GMT

owner changed from aron.carroll to toby
priority changed from blocker to major

Thanks Ross this is exactly what I wanted.

Just wondering about the use of the c.form variable to insert the form into the page. I know we do this with the dataset and groups to support legacy templates, do we need to do this going forward? I'd rather each action just rendered one template and leaves it up to that to render the rest.

Toby, there's a couple of TODO statements in the code where the breadcrumbs need fixing. I think the templates just need access to the package dict.

https://github.com/okfn/ckan/blob/feature-2375-demo-theme/ckan/templates/related/pages/form_page.html#L6 https://github.com/okfn/ckan/blob/feature-2375-demo-theme/ckan/templates/related/edit.html#L5

Also if you generate an error message on one of the forms (leave the title blank) the error string comes out wrapped in quotes. eg. u'title is missing'

Also might be of interest, I've converted the templates to use Jinja, and this is a good example of the conversion.

https://github.com/okfn/ckan/commit/d779addf6e48be67695b35c031393e2d90d8c22f

aron.carroll — Tue, 05 Jun 2012 15:46:30 GMT

Oh forgot to mention I've merged the code into feature-2375-demo-theme. We should delete the feature-2445-add-related-new branch.

toby — Wed, 06 Jun 2012 09:40:00 GMT

@aron,

this is as good a place as anywhere to answer your question about the c.form thing.

a) some of this we are sort of stuck with. datasets for example can be of different types and have different forms for different datasets. so they need to be rendered separately. I have no idea why or how much this is used but it exists as an option to complicate everything, not much we can do about it.

b) I think trying to do pages as a single rendering is not going to be easy because of the way ckan has been created maybe we can change this over time. One major issue for me is that ckan does lots of crazy stuff for some reasons but these reasons are not apparent from the code. Maybe there is some documentation somewhere but I'm not convinced.

anyhow we should maybe close this ticket now. You've added some stuff in the comment for me to do maybe that should be added as a new ticket?

status changed; resolution set

toby — Wed, 06 Jun 2012 11:39:14 GMT

status changed from assigned to closed
resolution set to fixed

closing as todos moved to other tickets and also completed