Book POST functionality by bdr99 · Pull Request #62 · gitenberg-dev/giten_site

bdr99 · 2018-01-31T21:16:25Z

This PR implements the functionality for POSTing new ebooks to the database.

To add books, send an HTTP POST request to /books/post/[secret]. The secret is read from a file called book-post-secret in the project root. The body of the POST request should be the YAML data for the book. If the book was successfully added to the database, the server will respond with a 200 status code.

To add books, send an HTTP POST request to /books/post/. The request should contain an HTTP header X-Gitenberg-Secret that contains the secret key. On the server side, this secret is read from an environment variable (GITENBERG_SECRET). The body of the POST request should be the YAML data for the book. If the book was successfully added to the database, the server will respond with a 200 status code.

eshellman

also may want to run pylint

eshellman · 2018-01-31T21:41:43Z

 from gitensite.apps.bookinfo.views import metadata

+#The secret must be stored in a file called "book-post-secret" located at the project root
+secretfile = open("book-post-secret", "rw")


The preferred way to pass secrets to the server on elastic beanstalk is via environment variables. look at how secrets are passed into the settings files. You can't just go in and write files!

Thanks, we will adjust the code to read the secret from an environment variable rather than from a file.

eshellman · 2018-01-31T21:48:20Z

    url(r'^all_repos.txt$', all_repos_txt, name='all_repos.txt'),
    url(r'^$', HomePageView.as_view(), name='home'),
    url(r'^books/(?P<book_id>\d+)\.(?P<ext>json|yaml)$', metadata, name='metadata'),
+    url(r'^books/post/' + secret, BookPostView.as_view(), name='book-post')


pass the secret into to view and let the view validate it. that way you can can get fancier later, or use the auth classes to handle multiple users.

OK, we will try to restructure the code to work this way.

In addition to changing the secret to be read from an environment variable, we also changed the secret to be passed to the view through an HTTP header.

eshellman · 2018-01-31T21:54:49Z

            return super(AjaxListView,self).get_queryset()
+
+class BookPostView(TemplateView):
+    @method_decorator(csrf_exempt)


I'm not familiar with method_decorator. usually I decorate a view directly in urls.py

We had to use the method decorator in order to disable Django's CSRF protection feature which was preventing us from implementing this POST endpoint. It is my understanding that the CSRF feature is meant to be used with HTML forms, so I don't think we need it.

You're correct; what I meant was that you can use csrf_exempt as a decorator directly.

Can you provide an example of what decorating a view in urls.py would look like?

in https://github.com/EbookFoundation/regluit/blob/master/frontend/urls.py#L108
csrf_exempt is in urls.py because the view is defined by an inherited method of a view class

in https://github.com/EbookFoundation/regluit/blob/master/api/views.py#L105
csrf_exempt decorates the view directly where it is defined

eshellman · 2018-01-31T21:57:23Z

+PyYAML.add_multi_constructor('!lcsh', default_ctor)
+
+def addBookFromYaml(yaml):
+    obj = PyYAML.load(yaml)


not safe_load() ?

Thanks, was not aware of safe_load. We will change this.

eshellman · 2018-01-31T22:02:15Z

+def addBookFromYaml(yaml):
+    obj = PyYAML.load(yaml)
+
+    (book,created) = Book.objects.get_or_create(book_id=int(obj['identifiers']['gutenberg']), repo_name=obj['_repo'])


book_id is declared unique, so this will throw exceptions if you have two repos with same book_id and different repo_name. Unfortunately these exist; also a problem if repo_names change

Ok, thanks for the warning. I guess we will have to find a different approach for the id. Maybe we will just use a numeric ID that increments each time a book is added.

I don't think you need a different approach, I think all you need to do is drop the unique declaration.

eshellman · 2018-02-05T19:45:33Z

I recommend reading this: https://simpleisbetterthancomplex.com/tutorial/2016/10/31/how-to-handle-github-webhooks-using-django.html

No shame in copy and paste.

eshellman · 2018-02-05T20:08:27Z

One thing I wish I'd done more is check my code style against a linter, like pylint. Now that I run pylint regularly, I find that it catches many of my coding errors and bad habits (such as having way too much code in a module.)

bdr99 · 2018-02-07T20:40:10Z

@eshellman Thanks for the link. We ended up using an approach similar to the one described on that page, in that the secret is passed through an HTTP header. I will edit the top comment to describe the new method.

… & description

eshellman · 2018-02-14T23:45:50Z

@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-


could you squash these migrations into one?
to do this:

migrate back to 0002

delete the 3 migrations (and the .pyc files!)

makemigrations

migrate

eshellman · 2018-02-14T23:50:50Z

    <div class="large-3 small-6 columns">
-        <h5 class="booktitle" style="font-weight: bold;">{{ book.title_short }}</h5>
-        <p class="bookauthor">{{ book.author }}</p>
+        <h5 class="booktitle" style="font-weight: bold;"><a href="/book/{{ book.book_id }}">{{ book.title_short }}</a></h5>


any reason not to use {% url %} tag?

eshellman · 2018-02-14T23:59:01Z

+        <div class="row">
+        {% for sameauthor_book in sameauthor %}
+            <div class="large-2 small-4 columns end">
+                <a href='/book/{{sameauthor_book.book_id}}'>


use url tag?

eshellman · 2018-02-15T00:01:11Z

        self.assertEqual(r.status_code, 200)
+
+        #Test to ensure that secret is in environment variables
+        self.assertEqual("GITENBERG_SECRET" in os.environ, True)


how are you going to get "GITENBERG_SECRET" into the test environment?

eshellman · 2018-02-15T00:04:59Z

+rights: Public domain in the USA.
+rights_url: http://creativecommons.org/about/pdm
+subjects:
+- '!lcsh: Love stories'


this is the wrong syntax.
wrong
- '!lcsh: Love stories'
right
- !lcsh: 'Love stories'

I see that this is copied from a gitberg test. Let me double check.

I fixed this
an example of correct syntax:

https://raw.githubusercontent.com/gitenberg-dev/metadata/master/samples/pandata.yaml

eshellman · 2018-02-15T00:13:56Z

                (book,created) = Book.objects.get_or_create(book_id=int(pg_id), repo_name=repo_name)
+
                metadata=BookMetadata(book,rdf_library=rdf_library, enrich=should_enrich)
+


it seems like the following code should duplicate the code that handles a post. so maybe better to put it on the model so both management command and post handler can call book.load_metadata(metadata)

… into book-post

# Conflicts: # gitensite/apps/bookinfo/management/commands/load_repos.py

bdr99 · 2018-02-21T21:57:26Z

OK, we have addressed the following:

Travis test should not require secret
use url tags
combine migrations into one
remove duplicated code in load_repos management command and db.py

eshellman · 2018-02-21T22:02:45Z

looks like tests fail because of travis not knowing the secret

eshellman · 2018-02-22T00:02:07Z

No, that's not it; a 405 means the server won't accept the POST. I bet it's related to the way you tried to do csrf_exempt; try wrapping the as_view method in urls.py

… into book-post

bdr99 · 2018-02-26T17:56:27Z

I got the tests working. The problem was that when the test client sent the HTTP POST request, it was being redirected because of the security settings in settings.py. And HTTP POST requests are not preserved when following redirects, so it was being changed to a GET request, which caused the 405 failure (Method not Allowed). I changed the test to use a secure request and it works now.

eshellman · 2018-02-26T19:45:21Z

Deployed on production. Now add the secret into the environment and you're ready to start posting books.

eshellman · 2018-02-26T19:48:47Z

or maybe the load_repos command needs running. let me know.

bdr99 · 2018-02-28T19:15:42Z

Yes, the load_repos command needs to be run so that the data can be loaded into the new schema.

Edit: It would be a good idea to merge #63 before doing this though, as we fixed a bug about loading the repo name.

bdr99 added 4 commits December 5, 2017 14:18

update Django data model to reflect the new database schema

04f87e0

inital commit for ebook listing pages

38968c3

update load_repos command to use the new database schema

a621b4e

add book posting functionality

ca91a08

eshellman requested changes Jan 31, 2018

View reviewed changes

bdr99 added 3 commits February 7, 2018 15:28

Store secret in an environment variable rather than a file

3d9f64e

use safe_load() instead of load() when parsing incoming books

6da65f9

make book_id not unique (some books have same book_ids)

628130a

bdr99 added 19 commits February 7, 2018 15:54

initial Travis file for testing

33e39da

add environment variables to travis file

bc3002c

fix environment variables

47d535c

add command to create django log

dfe8d65

use sudo when creating django log directory

2bca2ea

change permission on django log directory

1c88523

create separate settings file for travis

6cdd683

make travis settings a copy of regular settings

6ff2a07

add BOTO_CONFIG variable to fix travis bug

39a4e0b

import travis settings from settings.py

e1fda23

detect Travis in settings.py

7d1f5e2

follow redirects in tests

d815c48

add author property to the book model

ff7b972

add properties for truncated title and description

b2374b9

revised search results page - include author name and truncated title…

70b82ad

… & description

use scroll bar for descrption on search results page

f15b24b

add margin above search result rows

3384970

update Django data model to reflect the new database schema

55cc24f

update load_repos command to use the new database schema

81b933b

bdr99 added 2 commits February 14, 2018 16:30

Resolve merge conflicts

e9e84d3

restore accidentally modified settings.py

dca2179

eshellman reviewed Feb 14, 2018

View reviewed changes

eshellman reviewed Feb 15, 2018

View reviewed changes

bdr99 and others added 10 commits February 21, 2018 15:34

merge migrations into one

c58932c

make load_repos command use addBookFromYaml function

fde89c2

fix build

d551f35

use url tag

5699d82

correct the test yaml

2023915

don't require secret in test

318b7cd

Merge branch 'book-post' of https://github.com/gitenberg-dev/giten_site…

d2f91c0

… into book-post

follow redirects in test

c03ea4e

Merge branch 'master' into book-post

0e8e598

# Conflicts: # gitensite/apps/bookinfo/management/commands/load_repos.py

Merge remote-tracking branch 'origin/book-post' into book-post

e0c9a82

bdr99 added 3 commits February 26, 2018 12:33

fix ebook post test getting redirected

c4f955b

remove extraneous "print" statements

e69d67f

Merge branch 'book-post' of https://github.com/gitenberg-dev/giten_site…

ef3761e

… into book-post

eshellman approved these changes Feb 26, 2018

View reviewed changes

eshellman merged commit 0045ced into master Feb 26, 2018

		(book,created) = Book.objects.get_or_create(book_id=int(pg_id), repo_name=repo_name)

		metadata=BookMetadata(book,rdf_library=rdf_library, enrich=should_enrich)

Uh oh!

Uh oh!

Conversation

bdr99 commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eshellman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eshellman commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eshellman commented Feb 5, 2018

Uh oh!

bdr99 commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdr99 commented Feb 21, 2018

Uh oh!

eshellman commented Feb 21, 2018

Uh oh!

eshellman commented Feb 22, 2018

Uh oh!

bdr99 commented Feb 26, 2018

Uh oh!

eshellman commented Feb 26, 2018

Uh oh!

eshellman commented Feb 26, 2018

Uh oh!

bdr99 commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

bdr99 commented Jan 31, 2018 •

edited

Loading

eshellman commented Feb 5, 2018 •

edited

Loading

bdr99 commented Feb 7, 2018 •

edited

Loading

bdr99 commented Feb 28, 2018 •

edited

Loading