Ticket #2445 (closed enhancement: fixed)

Opened 2 years ago

Last modified 2 years ago

Create demo dataset add related page

Reported by: aron.carroll Owned by: toby
Priority: major Milestone: ckan-sprint-2012-06-25
Component: ckan Keywords: demo-theme
Cc: toby, ross Repository: ckan
Theme: none

Description (last modified by aron.carroll) (diff)

Discussion:

https://okfn.basecamphq.com/projects/9558659-demo-ckan-front-end/posts/62821386/comments

Implementation:

http://s031.okserver.org:2375/dataset/adur_district_spending/related/new

Needs a new endpoint at /dataset/DATASET/related/new this should have a form containing the following fields.

  • Title (required)
  • Type of item (Application|Visualisation)
  • Description
  • URL (required)
  • Image URL

When submitted if an item is created it should redirect (303) back to /dataset/DATASET/related with a flash message saying "Related item was successfully created".

If failed to create it should redirect (303) back to the form and populate the error messages, see add dataset for examples (https://github.com/okfn/ckan/blob/feature-2375-demo-theme/ckan/templates/package/new_package_form.html)

Change History

comment:1 Changed 2 years ago by aron.carroll

  • Milestone set to current-ckan-sprint-2012-05-29

comment:2 Changed 2 years ago by aron.carroll

  • Description modified (diff)

comment:3 Changed 2 years ago by aron.carroll

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

comment:4 Changed 2 years ago by aron.carroll

  • Cc ross added
  • Description modified (diff)

comment:5 Changed 2 years ago by ross

  • 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.

comment:6 Changed 2 years ago by toby

@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

comment:7 Changed 2 years ago by ross

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

comment:8 Changed 2 years ago by ross

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

comment:9 Changed 2 years ago by toby

@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

comment:10 Changed 2 years ago by ross

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

comment:11 Changed 2 years ago by aron.carroll

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.

comment:12 Changed 2 years ago by ross

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

Also templates in templates_legacy?

comment:13 Changed 2 years ago by toby

@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

comment:14 Changed 2 years ago by ross

  • 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.

comment:15 Changed 2 years ago by aron.carroll

  • 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

comment:16 Changed 2 years ago by aron.carroll

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

comment:17 Changed 2 years ago by toby

@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?

comment:18 Changed 2 years ago by toby

  • Status changed from assigned to closed
  • Resolution set to fixed

closing as todos moved to other tickets and also completed

Note: See TracTickets for help on using tickets.