Refactor/carousels #558

mekarpeles · 2017-09-09T05:19:12Z

[WIP] some obvious improvements need to be made, but should be feature complete and ready for QA

mekarpeles

self code review with changes required

mekarpeles · 2017-09-09T05:19:52Z

openlibrary/core/lending.py


+def get_available(limit=None, page=1, subject=None):
+    """Retrieves a list of available works for carousels from archive.org"""
+    url = "https://archive.org/advancedsearch.php?q=collection%3Ainlibrary"


should likely be move to a config, or at least use the archive.org variable

mekarpeles · 2017-09-09T05:20:26Z

openlibrary/core/lending.py

+    limit = limit or MAX_IA_LIMIT
+    if subject:
+        url += "+AND+openlibrary_subject%3A" + subject
+    url += "+AND+loans__status__status%3AAVAILABLE&fl%5B%5D=identifier&fl%5B%5D=openlibrary_edition&fl%5B%5D=openlibrary_work&&fl%5B%5D=openlibrary_edition&sort%5B%5D=&sort%5B%5D=&sort%5B%5D=&rows=" + str(limit) + "&page=" + str(page) + "&output=json"


consider moving params to a dict and urlencoding to make it a little more clear/readable what's going on here

mekarpeles · 2017-09-09T05:20:53Z

openlibrary/core/lending.py

    except Exception as e:
        return {'error': 'request_timeout'}

+def get_available(limit=None, page=1, subject=None):


Let's do better with docstrings

mekarpeles · 2017-09-09T05:21:57Z

openlibrary/plugins/openlibrary/api.py

-        } if books else {'error': 'out_of_range'}
-        return delegate.RawText(simplejson.dumps(result),
+    def GET(self):
+        i = web.input(limit=6, page=1)


6 should probably use a constant like ITEMS_PER_CAROUSEL_PAGE = 6

mekarpeles · 2017-09-09T05:22:43Z

openlibrary/plugins/openlibrary/home.py

-        return render_template(
+        page = render_template(
            "home/index", stats=stats,
            blog_posts=blog_posts,


unclear returncart_list is being used anywhere -- it's rendered from within the template.

mekarpeles · 2017-09-09T05:27:00Z

openlibrary/plugins/openlibrary/js/ol.js

-    var elemTop = $(elem).offset().top;
-    var elemBottom = elemTop + $(elem).height();
-    return ((docViewTop < elemTop) && (docViewBottom > elemBottom));
+    if ($(elem).offset()) {


this shouldn't really be in this code review, it should be on the refactor/slick-carousels branch (which updated OL to be ready for slick carousels). The upgrade of jquery makes $(elem) undefined, hence this patch. We might as well add it here. Note: Other things like manage bookcovers breaks w/ latest jquery.

mekarpeles · 2017-09-09T05:27:43Z

openlibrary/templates/home/carousel_test.html

@@ -0,0 +1,70 @@
+


XXX this should be in refactor/slick-carousels and removed from this branch

mekarpeles · 2017-09-09T05:28:03Z

openlibrary/templates/home/homepage.html

@@ -0,0 +1,6 @@
+<!-- Add the slick-theme.css if you want default styling -->


XXX this should be removed from this branch and added to refactor/slick-carousels

mekarpeles · 2017-09-09T05:29:56Z

openlibrary/templates/site.html

 $def with (page)

-$if not ctx.get("sent_head"):
+$if False and objhas(page, 'homepage') is True:


These changes should have been rolled back and moved to refactor/slick-carousels

mekarpeles · 2017-09-09T05:30:15Z

static/js/homepage.js

@@ -0,0 +1,85 @@
+var carousels;


This file should be on refactor/slick-carousels and removed from this branch

mekarpeles · 2017-09-09T06:17:41Z

@bfalling ready for review.
Please note, carousels (write large) are still painting "availability" using the v1 API and so some books which are actually available (such as those in the staff picks carousel) may appear checked out. We haven't pushed the enhanced editions code yet, so the editions page will appear to corroborate this story (that the book is checked out). Visiting archive.org will show the edition's true availability.

Will submit another branch shortly w/ migration of carousel ajax checking/painting from v1 to v2 in a bit.

bfalling

High-level review complete. Please see comments.

bfalling · 2017-09-11T22:33:50Z

openlibrary/core/lending.py

+def get_available(limit=None, page=1, subject=None):
+    """Experimental. Retrieves a list of available editions from
+    archive.org advancedsearch which are available, in the inlibrary
+    collection, and optionally apart of an `openlibrary_subject`.


bfalling · 2017-09-11T22:37:28Z

openlibrary/core/lending.py

+            if item.get('openlibrary_work'):
+                results[item['openlibrary_work']] = item['openlibrary_edition']
+
+        keys = web.ctx.site.things({"type": "/type/edition", "ocaid": results.values()})


It's correct that the "ocaid" value is a list, right?

bfalling · 2017-09-11T22:41:04Z

openlibrary/plugins/openlibrary/home.py

-            d.daisy_url = book.url("/daisy")

-        if 'lendinglibrary' in collections:
+        if 'lendinglibrary' in collections or 'inlibrary' in collections:


lendinglibraryis deprecated. You can remove any references to it. Lendable books are only in inlibrary.

bfalling · 2017-09-11T22:43:36Z

openlibrary/plugins/openlibrary/tests/test_home.py

+            "cover_url": ""
        }
+        #assert book['title'] in self.render(book)
+        #assert self.link_count(self.render(book)) == 1


Remove comments?

bfalling · 2017-09-11T22:44:13Z

openlibrary/plugins/openlibrary/tests/test_home.py

-        assert len(soup.findAll("li")) == 1
-        assert len(soup.findAll("a")) == 1
+        #assert len(soup.findAll("li")) == 1
+        #assert len(soup.findAll("a")) == 1


Remove comments?

bfalling · 2017-09-11T22:46:17Z

openlibrary/templates/books/carousel_item.html

        <div class="coverEbook">
-          <span class="actions read">
-            <a href="$book.borrow_url" title="$_('Read this book')" class="borrow-link"
+            <a href="$(book.get('inlibrary_borrow_url') or book.get('borrow_url'))" title="$_('Borrow this book')" class="borrow-link"


book.get('inlibrary_borrow_url') or book.get('borrow_url') is also used up above. Seems brittle. What about extracting this out to a well-named helper variable, and using the variable in these locations instead?

bfalling · 2017-09-11T22:47:02Z

openlibrary/templates/borrow/read.html

+<div class="section">
+    <form method="get" action="/search" class="siteSearch olform">
+    <label for="q" class="hidden">Keywords</label>
+    <input type="text" name="q" id="q" value="" size="100" style="width:505px;"/>


Why the inline style?

bfalling · 2017-09-11T22:50:24Z

openlibrary/plugins/openlibrary/tests/test_home.py

+        #del book['authors']
+        #assert book['title'] in self.render(book)
+        #assert self.link_count(self.render(book)) == 1



(Randomly placed comment) I see the tests that were removed to make this all work. But I didn't see any new tests added. Are we covered?

I was looking for new tests too :) It would be great to have tests to describe and document the expected behaviour of new features.

…rk present

…branch

…l books now

…ssues on timeout

…r logic

mekarpeles · 2017-09-29T03:28:51Z

Fixed click tracking + memcached, got lgtm from @bfalling

mekarpeles commented Sep 9, 2017

View reviewed changes

mekarpeles force-pushed the refactor/carousels branch from b73f135 to 9a95de5 Compare September 9, 2017 14:40

bfalling requested changes Sep 11, 2017

View reviewed changes

mekarpeles force-pushed the refactor/carousels branch from 01c657f to ead1003 Compare September 25, 2017 22:38

mekarpeles added 25 commits September 28, 2017 20:49

demo of carousel feature

78413a1

experimenting with slick carousels which fetch from archive.org

2e370da

replacing slow popular books carousel w/ performant staff picks

f8c1ae9

removing legacy code and updating test

5044a5b

fixing cover_url changes to fallback from work to edition where no wo…

c1608a0

…rk present

removing code which belongs on experimental refactor/slick-carousels …

68d5cea

…branch

adds caching to staff picks, adds constants

3a9aef9

migrates carousels to use availability v2 api

437f1d4

fixes my loans copy, styles carousels

703417c

choosing better carousel control colors

83ddc05

choosing better check availability btn color

55ff351

fixed staff picks carousel to use editions instead of ocaid; shows al…

0e664a9

…l books now

fixing carousel height to show full cover

2eb395c

removing deprecated functions, uncaching staff picks due to strange i…

c9d963c

…ssues on timeout

Repairing tests

45cfb1c

making subject carousels render consistently with homepage carousels

b5bd247

moving waitlist logic to use availability v2 api

d09d472

updating carousel styles

0f4eb21

generic carousels and for-dummies ia-backed pilot

2140cd7

fixed homepage test cases, made returncart a generic carousel

53c56ea

adding technical books, adding tests, book cover rounded corners

15955bc

new homepage carousel styling + making carousel creation generic

873781a

fixing homepage carousel tests

22c41f0

adding back missing clearfix on subjects

a59c922

fixing duplicate IA tool import (breaking tests)

05a16be

mekarpeles added 3 commits September 28, 2017 20:49

sorting tech books by publish date to be more relevant

7d63b08

adding explicit memcache key names

ec33b22

total redesign of memcached ia carousels

8605cc4

mekarpeles force-pushed the refactor/carousels branch from f9c6dfb to 8605cc4 Compare September 29, 2017 03:05

mekarpeles added 2 commits September 29, 2017 03:06

changing the name of the memcached ia_book_carousel prefix

3e6ae70

Fixing click tracking for carousels + cleaning up carousel_item rende…

74d07c6

…r logic

mekarpeles merged commit 76ce8bb into master Sep 29, 2017

mekarpeles deleted the refactor/carousels branch April 22, 2019 01:49

		@@ -0,0 +1,6 @@
		<!-- Add the slick-theme.css if you want default styling -->

Uh oh!

Refactor/carousels #558

Refactor/carousels #558

Uh oh!

Conversation

mekarpeles commented Sep 9, 2017

Uh oh!

mekarpeles 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!

mekarpeles commented Sep 9, 2017

Uh oh!

bfalling 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!

mekarpeles commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants