-
-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify reviews for Common MB-BB entities #464
Conversation
391bbd4
to
82d122a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small improvements that I think we can make on this page.
One thing I was thinking about is that we have relationships in BB pointing to MB ids, but we also have relationships in MB pointing to BB pages. Do you think we should check both of these databases in this method, or should we instead build a report to help editors sync the list between MB and BB?
critiquebrainz/frontend/external/bookbrainz_db/common_entity.py
Outdated
Show resolved
Hide resolved
critiquebrainz/frontend/external/bookbrainz_db/common_entity.py
Outdated
Show resolved
Hide resolved
critiquebrainz/frontend/external/bookbrainz_db/common_entity.py
Outdated
Show resolved
Hide resolved
critiquebrainz/frontend/external/bookbrainz_db/common_entity.py
Outdated
Show resolved
Hide resolved
critiquebrainz/frontend/external/bookbrainz_db/common_entity.py
Outdated
Show resolved
Hide resolved
@@ -92,6 +92,40 @@ <h4 style="margin-bottom:0;">{{ _('Reviews') }}</h4> | |||
</ul> | |||
</div> | |||
{% endif %} | |||
|
|||
{% if author_info is defined and author_info %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small thing here, when we know that the "empty" case is an empty list, we don't necessarily need and author_info
, because {% for author in author_info %}
will be an empty list and won't loop through anything.
However, this being said, I think that we could do this a bit differently:
{% if author_info is defined and author_info %} | |
{% if author_info is defined and author_info %} | |
<h4> {{ _('This entity is also in the BookBrainz Database as ') }} | |
{% for author in author_info %} |
Because if for some reason there are two related authors, we will show this heading twice. We should only do it once.
588981c
to
7f07b3b
Compare
This PR unifies reviews for Artist-Author, Work-Literary Work having relationship between them.