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-359: Implemented the reviewal of labels #264

Merged
merged 11 commits into from Aug 15, 2019

Conversation

@spellew
Copy link
Contributor

commented Jun 16, 2019

No description provided.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

@spellew When you have time, please update this PR to fix conflicts too. Thanks! :)

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@ferbncode Resolved.

#
# You should have received a copy of the GNU General Public License along
# with this program; if not, write to the Free Software Foundation, Inc.,
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 5, 2019

Collaborator

I don't think we need a license here.

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 5, 2019

Collaborator

(Also for other remaining pull requests)

import critiquebrainz.frontend.external.musicbrainz_db.exceptions as mb_exceptions
# import critiquebrainz.db.review as db_review
# from critiquebrainz.frontend.forms.rate import RatingEditForm
# from critiquebrainz.frontend.views import get_avg_rating

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 5, 2019

Collaborator

Let's remove the commented imports

@ferbncode
Copy link
Collaborator

left a comment

The pull request is good, but let's try to

  1. Fix the conflicts
  2. Show some data on the label page for the Label. Right now it is empty. (Atleast the name, maybe the annotations and the releases too). See, for example: https://musicbrainz.org/label/5899271b-a77a-4081-93a9-0bd05b9b4180

@spellew spellew force-pushed the spellew:label-entity-support branch from c531f95 to 3adc216 Aug 8, 2019

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@ferbncode Updated the entity page to display some data.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

@spellew Right now, the information is okay, One thing I noticed for artists and labels is missing rating which is also part of the proposal. Please update this PR for including ratings. I'll add review for the code later today 👍

entity_type='label',
user_id=current_user.id,
)
my_review = my_reviews[0] if my_count else None

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 13, 2019

Collaborator

What happens if I have a review already for a label. It should be passed to the render_template to display the option of Edit Review rather than Write Review. See place.py or release_group.py for example.

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 13, 2019

Collaborator

Also, other important variable used in the template like current_user.

This comment has been minimized.

Copy link
@spellew

spellew Aug 13, 2019

Author Contributor

All the other important variables too? I think I only passed the ones that we're currently using, to the render template.

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 13, 2019

Collaborator

See the base.py (the variable current_user and my_review do play a role there for displaying different button labels, for example

This comment has been minimized.

Copy link
@spellew

spellew Aug 14, 2019

Author Contributor

I've added current_user and my_review those seem to be the only variables used outside the label template.

@@ -53,6 +53,13 @@
{{ _('Artist') }}
</span>
{% endif %}
{% elif entity_type == 'label' %}
<img src="{{ get_static_path('images/placeholder_place.svg') }}" {{ attributes }} />

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 13, 2019

Collaborator

placeholder_disc.svg would be much relating to labels, imho.

This comment has been minimized.

Copy link
@spellew

spellew Aug 14, 2019

Author Contributor

Updated.

@ferbncode
Copy link
Collaborator

left a comment

I also would prefer to show the releases from the label on the entity page, but let's make it an enhancement feature which we can work on after the other entities. Wdyt?

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@ferbncode I was planning on showing the releases, but I'm worried about time constraints, and if that can be achieved while we're working on other entities.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

@spellew Yes, that is my plan to do it after all the entities as an enhancement (Right now, I'll just create a ticket before merging this)

@paramsingh

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Did you create a ticket already, @ferbncode ?

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

@paramsingh I'll make sure to create it today.

@ferbncode
Copy link
Collaborator

left a comment

@spellew This looks good. Let's merge this. Also, here is the ticket link: https://tickets.metabrainz.org/browse/CB-359

@spellew spellew changed the title CB-270: Implemented the reviewal of labels CB-359: Implemented the reviewal of labels Aug 15, 2019

@paramsingh paramsingh merged commit 7064672 into metabrainz:master Aug 15, 2019

2 checks passed

Jenkins [PyLint & Flake8] Build finished.
Details
Jenkins [PyTest] Build finished.
Details
@@ -1 +1,2 @@
ALTER TYPE entity_types ADD VALUE 'artist' AFTER 'place';
ALTER TYPE entity_types ADD VALUE 'label' AFTER 'artist';

This comment has been minimized.

Copy link
@paramsingh

paramsingh Aug 15, 2019

Member

I just saw this, this should be in a different script so I can run it on the main db. Generally, each PR should have a single schema change script.

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 15, 2019

Collaborator

Ah, right. @spellew Please add schema change scripts to admin/schema_changes (One for artists and one for labels)

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 15, 2019

Collaborator

Thank you @paramsingh for pointing that out :)

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 15, 2019

Collaborator

And for fixing it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.