CB-84: Add checkbox to filter releases with reviews on artist discography pages. #153

ferbncode · 2017-08-22T21:10:31Z

Added checkbox on artist discography pages to filter only those releases (on that page) which have reviews.

To fetch only those releases which have a review (rather than filtering on that page), I guess we can later fetch all releases (rather than applying the limit) and then show them (shouldn't slow down the page).

ferbncode · 2017-08-22T21:18:40Z

Some screenshots:
Unchecked:

Checked for showing only reviewed entities:

No reviewed entities found:

alastair · 2017-08-23T09:01:38Z

critiquebrainz/frontend/templates/artist/entity.html

+      <div class="col-sm-4">
+        <div>
+          <input type="checkbox" name="only-reviewed" value="1" class="only-reviewed" onchange="showOnlyReviewed()"/>
+          {{ _('Filter releases here with reviews') }}<br/>


"Only show releases with reviews"

alastair · 2017-08-23T09:02:25Z

critiquebrainz/frontend/templates/artist/entity.html

@@ -66,13 +76,16 @@
        <a href="{{ url_for('artist.entity', mbid=artist.id) }}?release_type=other">{{ _('Other releases') }}</a>
      </li>
    </ul>
+    <div class="no_reviewed_entities">


if the default is to not check the box, can you make this hidden by default instead of hiding it in js?

alastair · 2017-08-23T09:11:13Z

critiquebrainz/frontend/templates/artist/entity.html

+      </div>
+      <div class="col-sm-4">
+        <div>
+          <input type="checkbox" name="only-reviewed" value="1" class="only-reviewed" onchange="showOnlyReviewed()"/>


instead of using an html attribute here, other places in the CB code us jquery to install an event handler:

critiquebrainz/critiquebrainz/frontend/templates/review/modify/base.html

Line 76 in 4835946

$("#btn-publish").click(function(){

We should do the same thing for consistency

alastair · 2017-08-23T09:14:50Z

critiquebrainz/frontend/templates/artist/entity.html

+{% block scripts %}
+  {{ super() }}
+  <script>
+  $(".no_reviewed_entities").hide();


With javascript it's a good idea to run commands only after the full page has loaded. You can use document.ready here:

critiquebrainz/critiquebrainz/frontend/templates/review/modify/base.html

Line 63 in 4835946

$(document).ready(function() {

alastair

👍 OK by me, pending feedback from @gentlecat

gentlecat · 2017-08-29T19:35:05Z

critiquebrainz/frontend/templates/artist/entity.html

+      <div class="col-sm-4">
+        <div>
+          <input type="checkbox" name="only-reviewed" value="1" id="only-reviewed"/>
+          {{ _('Only show releases with reviews') }}<br/>


Artist page doesn't have releases. Only release groups.

gentlecat · 2017-08-29T19:36:01Z

critiquebrainz/frontend/templates/artist/entity.html

+        }
+      })
+    })
+  </script>


It's better to put this JS code into a separate script in static/scripts, build and import it.

gentlecat · 2017-08-31T12:55:00Z

Also please don't forget to assign yourself to the issue in JIRA and mark it as WIP so that there's no confusion if someone else decides to work on it.

gentlecat · 2017-08-31T12:57:06Z

On the last screenshot there's a "Next" button. Is that a bug or the screenshot is just a prototype?

ferbncode · 2017-09-02T17:20:01Z

On the last screenshot there's a "Next" button. Is that a bug or the screenshot is just a prototype?

The "Next" Button appeared as the filtering was being done on just that page. So, as there were no more reviewed entities (on that page), the "Next Button" appeared. I've fixed this and now all reviewed release groups are shown together.

I've rebased the present branch and based it on PR #140. I've modified the code to fix the problem by fetching all release groups of an artist by directly using the db.
Now all reviewed release groups are shown once the filter is checked. Also, the Previous and the Next buttons don't reload the entire page, but they just refresh the entities to be shown.

to single pages Use direct db access, make use of js for next, previous links

gentlecat · 2017-09-03T15:59:39Z

Was trying to test this and ran into the following issue:

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/werkzeug/wsgi.py", line 660, in __call__
    return app(environ, start_response)
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1997, in __call__
    return self.wsgi_app(environ, start_response)
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1985, in wsgi_app
    response = self.handle_exception(e)
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1540, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.6/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.6/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/code/critiquebrainz/frontend/views/release_group.py", line 28, in entity
    release = mb_release.get_release_by_id(release_group['release-list'][0]['id'])
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/release.py", line 23, in get_release_by_id
    release = _get_release_by_id(mbid)
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/release.py", line 31, in _get_release_by_id
    includes=['media', 'release-groups'],
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/release.py", line 82, in fetch_multiple_releases
    releases = {str(mbid): to_dict_releases(releases[mbid], includes_data[releases[mbid].id]) for mbid in mbids}
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/release.py", line 82, in <dictcomp>
    releases = {str(mbid): to_dict_releases(releases[mbid], includes_data[releases[mbid].id]) for mbid in mbids}
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/serialize.py", line 193, in to_dict_releases
    for medium in includes['media']]
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/serialize.py", line 193, in <listcomp>
    for medium in includes['media']]
  File "/code/critiquebrainz/frontend/external/musicbrainz_db/serialize.py", line 153, in to_dict_medium
    'format': medium.format.name,
AttributeError: 'NoneType' object has no attribute 'name'

gentlecat · 2017-09-03T16:10:03Z

I'm seeing this on master as well.

gentlecat

The main problem here is that you removed pagination from the release group retrieval. Even if most artists don't have many, some pages will probably be slowed down significantly the more release groups they have.

If this is not the case, I'd like to at least see some proof.

ferbncode · 2017-09-03T20:20:31Z

Even if most artists don't have many, some pages will probably be slowed down significantly the more release groups they have.

That is correct. Some pages slow down as they have many release groups (Various Artists page (no. of release groups: 145133), for instance). But fetching all release groups is important if all reviewed release groups are to be shown together. Some possible (though probably not the best) work-arounds on frontend can be:

adding a loader for making the user wait for the page for load.
keeping pagination and filtering the reviewed entities only on that very page.

I am unsure if either is a good choice. Please suggest. Thanks!

paramsingh · 2018-02-16T18:52:35Z

@ferbncode, I'd like to discuss how exactly to move forward on this, once you get the time.

paramsingh · 2018-02-19T07:51:02Z

Closing this for now so that discussion can be moved to the ticket: https://tickets.metabrainz.org/browse/CB-84?focusedCommentId=45384&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-45384

alastair requested changes Aug 23, 2017

View reviewed changes

alastair approved these changes Aug 29, 2017

View reviewed changes

gentlecat suggested changes Aug 29, 2017

View reviewed changes

ferbncode force-pushed the cb-84 branch from a29b4f1 to a6956ee Compare September 2, 2017 17:17

ferbncode added 4 commits September 3, 2017 12:24

CB-84: Add checkbox to filter releases with reviews

6af9031

Modify code for consistency, use document.ready, other fixes

8f89531

Put the js code in a separate file, other fixes

e5d8499

Show all reviewed releases rather than limiting the filtering

1b2459f

to single pages Use direct db access, make use of js for next, previous links

ferbncode force-pushed the cb-84 branch from a6956ee to 1b2459f Compare September 3, 2017 07:24

gentlecat suggested changes Sep 3, 2017

View reviewed changes

gentlecat added the needs work label Sep 3, 2017

paramsingh closed this Feb 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-84: Add checkbox to filter releases with reviews on artist discography pages. #153

CB-84: Add checkbox to filter releases with reviews on artist discography pages. #153

ferbncode commented Aug 22, 2017 •

edited

ferbncode commented Aug 22, 2017 •

edited

alastair Aug 23, 2017

alastair Aug 23, 2017

alastair Aug 23, 2017

alastair Aug 23, 2017

alastair left a comment

gentlecat Aug 29, 2017

gentlecat Aug 29, 2017

gentlecat commented Aug 31, 2017

gentlecat commented Aug 31, 2017

ferbncode commented Sep 2, 2017 •

edited

gentlecat commented Sep 3, 2017

gentlecat commented Sep 3, 2017

gentlecat left a comment

ferbncode commented Sep 3, 2017 •

edited

paramsingh commented Feb 16, 2018

paramsingh commented Feb 19, 2018

CB-84: Add checkbox to filter releases with reviews on artist discography pages. #153

CB-84: Add checkbox to filter releases with reviews on artist discography pages. #153

Conversation

ferbncode commented Aug 22, 2017 • edited

ferbncode commented Aug 22, 2017 • edited

alastair Aug 23, 2017

Choose a reason for hiding this comment

alastair Aug 23, 2017

Choose a reason for hiding this comment

alastair Aug 23, 2017

Choose a reason for hiding this comment

alastair Aug 23, 2017

Choose a reason for hiding this comment

alastair left a comment

Choose a reason for hiding this comment

gentlecat Aug 29, 2017

Choose a reason for hiding this comment

gentlecat Aug 29, 2017

Choose a reason for hiding this comment

gentlecat commented Aug 31, 2017

gentlecat commented Aug 31, 2017

ferbncode commented Sep 2, 2017 • edited

gentlecat commented Sep 3, 2017

gentlecat commented Sep 3, 2017

gentlecat left a comment

Choose a reason for hiding this comment

ferbncode commented Sep 3, 2017 • edited

paramsingh commented Feb 16, 2018

paramsingh commented Feb 19, 2018

ferbncode commented Aug 22, 2017 •

edited

ferbncode commented Aug 22, 2017 •

edited

ferbncode commented Sep 2, 2017 •

edited

ferbncode commented Sep 3, 2017 •

edited