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

CB-270: Implemented the reviewal of recordings #265

Open
wants to merge 20 commits into
base: master
from

Conversation

@spellew
Copy link
Contributor

commented Jun 16, 2019

No description provided.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

@spellew Let's rebase this/fix conflicts and add ratings for recordings too. (Also, when you're done, could you post on how the recording page looks like). Thanks! :)

@spellew spellew force-pushed the spellew:recording-entity-support branch from 80a9cf8 to 625d03f Aug 15, 2019

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@ferbncode Updated this pull request. Here is what the recording page currently looks like.

Screenshot_2019-08-15 Recording Mystery of Iniquity by - CritiqueBrainz

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Aug 18, 2019

@spellew Thanks!, that looks good :), do you think we can link recordings from the release-group pages to their entity (recording) pages?

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

@ferbncode It's already been done, but I'll remove the commented out AcousticBrainz link.
https://github.com/metabrainz/critiquebrainz/pull/265/files#diff-69f670bbf80bf4c3f59512c883416826R116

@ferbncode
Copy link
Collaborator

left a comment

image

The entity name is not shown successfully on the profile page of the reviewer. Please fix this formatting in the template for the user page. (frontend/templates/user/reviews.html)

@spellew This needs to be fixed for all new entities.

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@ferbncode Updated to include the titles for this entity and the ones that were merged.

[mbid],
includes=['artists', 'work-rels', 'url-rels'],
).get(mbid)
recording.update({'length': recording['length'] * 1000.0})

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 21, 2019

Collaborator

@spellew Why do you need to multiply by 1000? Also, I see that similar change was made in work/entity.html. Could you move both calls to a either a template function or a python function. We should try to stay consistent in order to reduce changes in the future.

This comment has been minimized.

Copy link
@spellew

spellew Aug 22, 2019

Author Contributor

Brainzutils returns the length in seconds, but we need milliseconds for the track_length function.

This comment has been minimized.

Copy link
@spellew

spellew Aug 22, 2019

Author Contributor

Updated here and in the pull request regarding works.

else:
external_reviews = []

limit = int(request.args.get('limit', default=10))

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 21, 2019

Collaborator

Could you also move the default to a config value?

This comment has been minimized.

Copy link
@spellew

spellew Aug 22, 2019

Author Contributor

Updated.

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@ferbncode Could you take a look at why Jenkins failed?

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2019

@brainzbot retest this please

@ferbncode
Copy link
Collaborator

left a comment

@spellew The PR looks good. Please check why the recording length on the page is not correct, and the artist name is absent. (Might be the local database I have, but then the recording length should not be 00:00)
image

spellew added 2 commits Aug 24, 2019
@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

@ferbncode Updated.

@ferbncode
Copy link
Collaborator

left a comment

The PR is good, let's merge this after a small change. (Also, please remember to update the PR to fix the brainzutils-python version later. :)

{{ _('%(recording)s', recording=recording_name) }}

{% if recording['life-span'] %}
<small>{{ recording['life-span']['begin'][:4] }}</small>

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 25, 2019

Collaborator

Is this for the year of the recording? Please move this as a function in the template just so that it gets easier to read the code. :)

This comment has been minimized.

Copy link
@spellew

spellew Aug 27, 2019

Author Contributor

I believe this was accidentally included, since recordings don't have life-spans.

spellew added 2 commits Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.