Skip to content
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

Merged
merged 4 commits into from
Oct 25, 2022

Conversation

anshg1214
Copy link
Member

@anshg1214 anshg1214 commented Aug 25, 2022

This PR unifies reviews for Artist-Author, Work-Literary Work having relationship between them.

@anshg1214 anshg1214 force-pushed the common-bb-mb-entiites branch from 391bbd4 to 82d122a Compare September 9, 2022 06:55
Copy link
Collaborator

@alastair alastair left a 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?

@@ -92,6 +92,40 @@ <h4 style="margin-bottom:0;">{{ _('Reviews') }}</h4>
</ul>
</div>
{% endif %}

{% if author_info is defined and author_info %}
Copy link
Collaborator

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:

Suggested change
{% 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.

critiquebrainz/frontend/views/bb_author.py Show resolved Hide resolved
@anshg1214 anshg1214 force-pushed the common-bb-mb-entiites branch from 588981c to 7f07b3b Compare September 26, 2022 15:54
@alastair alastair merged commit c497292 into metabrainz:master Oct 25, 2022
@github-actions
Copy link

Unit Test Results

    1 files  ±0      1 suites  ±0   2m 25s ⏱️ ±0s
201 tests ±0  201 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit c497292. ± Comparison against base commit c497292.

@anshg1214 anshg1214 deleted the common-bb-mb-entiites branch October 25, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants