Refactor textbooks to use locator URLs

[singingwolfboy]

Tests are passing. @dmitchell @cahrens could you review?

[cahrens]

I'm trying to understand why there are 3 different methods.

Why isn't the case of "new" handled by doing a PUT (or is it POST?) to textbooks_detail_handler with no ID? Isn't that the standard way of doing it?

[dmitchell]

+1 on @cahrens question

btw: I really think one of us needs to bite the bullet and remove the Backbone.HTTPemulate setting so it uses POST for create and PUT for update and then clean up our handlers to treat POST and PUT differently, but it seems like such a chore, that I've put it off just making it even more work.

[singingwolfboy]

When I developed the textbooks feature, I noticed that the list of textbooks was stored in one Mongo document, so getting/updating all textbooks at once was just as easy as getting/updating a single textbook at a time, and less error prone (because I could just overwrite the old list with the new content). As a result, I delegated data persistence to the Backbone collection, which would POST the entire collection of textbook models to the server in one HTTP request. Since this was acting on an entire collection, rather than one resource in the collection, I thought it made sense to POST to the textbooks_list_handler endpoint, and have a separate URL for creating new textbooks.

However, coming back to it now, I'm not sure that was a good idea -- it's better to be consistent. Perhaps I could make POST create a new resource, and PUT update the collection. Then I could delete the "new" handler entirely. Would that make sense?

[cahrens]

What would the relationship with textbooks_detail_handler be? Can we boil it all down to a single method?

GET with no index returns collection
GET with index returns just that textbook
POST with no index creates a textbook
PUT/POST with index updates the textbook

[singingwolfboy]

I think it's clearer to have two methods -- one that operates on collections, and one that operates on individual resources. Do you think it would be better to have one method that does both? These methods are big enough already -- I'm worried about stuffing more and more functionality into a single method.

[cahrens]

We've combined them in the past, but I don't feel that strongly about it. My main question is where does "new" belong if there are 2 methods--- with the collection method, or with the individual method? My first inclination is that it belongs with the individual method.

[singingwolfboy]

It can only belong with the individual method if the request creating the textbook already has an ID in mind for that textbook: if that's the case, then POSTing to the URL that corresponds to that ID will create the textbook if it doesn't already exist, and update it if it does. If the client doesn't have an ID in mind for the textbook yet, but wants the server to assign an ID, then it needs to use the collection method; it can't address the individual method without an ID.

[dmitchell]

Backbone's model is to POST w/o an id to imply create v using an id w/ PUT to imply update.

[cahrens]

I appreciate leaving these textbook methods in course.py for the purpose of the code review (diff), However, after the review is complete, could you move them to their own view file? course.py is too big,

[dmitchell]

put new as the 2nd arg to url_reverse: self.course_locator.url_reverse('textbooks','new')

[cahrens]

Except that we want to get rid of the need to put "new" at all and just key off the fact that there is no ID.

[cahrens]

Check out pylint warnings:

cms/djangoapps/contentstore/views/course.py 97.7%
63: C0111: (missing-docstring), get_locator_and_course: Missing docstring
318: W0612: (unused-variable), course_info_handler: Unused variable 'locator'
745: W0612: (unused-variable), textbooks_detail_handler: Unused variable 'locator'

[cahrens]

Why does the same variable need to be passed with 2 different names?

[singingwolfboy]

I changed it so that the template only references a textbooks variable, which I could pass in specifically. The context_course variable is used by an inherited template to render the dropdown menus -- it's not used in textbooks.html.

[cahrens]

It's hard for me to tell exactly what you changed due to the commit cleanup, but the things I looked at are good. Did not check out the branch and test again. 👍