codeforamerica / pdfhook

A Python web application for converting PDF forms into PDF-filling APIs
https://pdfhook.herokuapp.com
MIT License
46 stars 24 forks source link

Add a way to delete uploaded forms #73

Closed bengolder closed 8 years ago

brennv commented 8 years ago

Added archiving with https://github.com/codeforamerica/pdfhook/pull/77 by tagging the record as is_archived=True and removing archived records from the main page.

Once we have an 'admin view' maybe they should get full delete powers?

brennv commented 8 years ago

screen shot 2016-05-26 at 3 06 40 pm

bengolder commented 8 years ago

I think the "Delete" link is a great way to implement this on the HTML front end. Via the API, should we have a URL path like this? /<form_id>/delete/ Or should it just handle a DELETE HTTP request at /<form_id>/, since DELETE is an existing HTTP verb? http://www.restapitutorial.com/lessons/httpmethods.html Maybe it should return the JSON representation of the deleted form resource. I'm leaning towards the HTTP verb, so that this matches existing REST standards.

brennv commented 8 years ago

https://github.com/codeforamerica/pdfhook/pull/79 Using the url path for now, not looking forward to writing http method overrides. Checking out flask-restful for separating out all the pure api functions from view functions

bengolder commented 8 years ago

@brennv good call. I hadn't considered the difficulty of integrating it into a form

HTML4 and XHTML only specify POST and GET as HTTP methods that forms can use

It seems like it would be trivial to add the DELETE verb to the API. But that can be a separate task.

brennv commented 8 years ago

Playing with MethodViews in an effort align things closer to REST and start separating html from api views https://github.com/brennv/pdfhook/blob/refactor-views/src/pdfhook/views.py

bengolder commented 8 years ago

I like class-based views stylistically, and I'm moving that way in other projects that I'm working on. But I think in this case keeping the views as simple, function-based views would be more beginner-friendly. Most of the getting started examples in Flask all use function-based views with decorators.

We can achieve similar functionality by using the methods argument in blueprint.route(). For example:

@blueprint.route('/<int:pdf_id>/', methods=['DELETE'])
@blueprint.route('/<int:pdf_id>/delete/', methods=['POST'])
def delete_pdf(pdf_id):
    pdf = models.PDFForm.query.filter_by(id=pdf_id).delete()
    db.session.commit()
    return redirect(url_for('pdfhook.index'))

This way we don't have to maintain separate view functions for the API vs. the HTML form. Instead, we are just adding additional url routes to the same function.

brennv commented 8 years ago

Sounds good, added it to https://github.com/codeforamerica/pdfhook/pull/79