PR#1449: frontend: allow users to upvote or downvote projects - copr/copr

copr / copr

#1449 frontend: allow users to upvote or downvote projects

Merged 3 years ago by praiskup. Opened 3 years ago by frostyx.

copr/ frostyx/copr upvote-downvte-projects into master

frontend: show voting and project name as one block

Jakub Kadlcik • 3 years ago

410e873

frontend: allow users to upvote or downvote projects

Jakub Kadlcik • 3 years ago

2d9bdbe

frontend/coprs_frontend/alembic/versions/de903581465c_create_table_for_upvoting_and_.py

file added

+34

		`@@ -0,0 +1,34 @@`
		`+ """`
		`+ Create table for upvoting and downvoting projects`
		`+`
		`+ Revision ID: de903581465c`
		`+ Revises: 484a1d4dd424`
		`+ Create Date: 2020-07-26 19:36:51.199148`
		`+ """`
		`+`
		`+ import sqlalchemy as sa`
		`+ from alembic import op`
		`+`
		`+`
		`+ revision = 'de903581465c'`
		`+ down_revision = '484a1d4dd424'`
		`+`
		`+ def upgrade():`
		`+ op.create_table(`
		`+ 'copr_score',`
		`+ sa.Column('id', sa.Integer(), nullable=False),`
		`+ sa.Column('copr_id', sa.Integer(), nullable=False),`
		`+ sa.Column('user_id', sa.Integer(), nullable=False),`
		`+ sa.Column('score', sa.Integer(), nullable=False),`
		`+ sa.ForeignKeyConstraint(['copr_id'], ['copr.id'], ),`
		`+ sa.ForeignKeyConstraint(['user_id'], ['user.id'], ),`
		`+ sa.PrimaryKeyConstraint('id')`
		`+ )`
		`+ op.create_index(op.f('ix_copr_score_copr_id'), 'copr_score', ['copr_id'], unique=False)`
		`+ op.create_unique_constraint('copr_score_copr_id_user_id_uniq', 'copr_score', ['copr_id', 'user_id'])`
		`+`
		`+`
		`+ def downgrade():`
		`+ op.drop_constraint('copr_score_copr_id_user_id_uniq', 'copr_score', type_='unique')`
		`+ op.drop_index(op.f('ix_copr_score_copr_id'), table_name='copr_score')`
		`+ op.drop_table('copr_score')`

frontend/coprs_frontend/coprs/forms.py

file modified

		`@@ -1226,6 +1226,15 @@`
		`return True`


		`+ class VoteForCopr(FlaskForm):`
		`+ """`
		`+ Form for upvoting and downvoting projects`
		`+ """`
		`+ upvote = wtforms.SubmitField("Upvote")`
		`+ downvote = wtforms.SubmitField("Downvote")`
		`+ reset = wtforms.SubmitField("Reset vote")`
		`+`
		`+`
		`class AdminPlaygroundForm(FlaskForm):`
		`playground = wtforms.BooleanField("Playground", false_values=FALSE_VALUES)`

frontend/coprs_frontend/coprs/logic/coprs_logic.py

file modified

+36

		`@@ -811,6 +811,42 @@`
		`return query.filter(models.CoprChroot.delete_after < datetime.datetime.now())`


		`+ class CoprScoreLogic:`
		`+ """`
		`+ Class for logic regarding upvoting and downvoting projects`
		`+ """`
		`+`
		`+ @classmethod`
		`+ def get(cls, copr, user):`
		`+ query = db.session.query(models.CoprScore)`
		`+ query = query.filter(models.CoprScore.copr_id == copr.id)`
		`+ query = query.filter(models.CoprScore.user_id == user.id)`
		`+ return query`
		`+`
		`+ @classmethod`
		`+ def upvote(cls, copr):`
		`+ return cls.vote(copr, 1)`
		`+`
		`+ @classmethod`
		`+ def downvote(cls, copr):`
		`+ return cls.vote(copr, -1)`
		`+`
		`+ @classmethod`
		`+ def vote(cls, copr, value):`
		`+ """`
		+ Low-level function for giving score to projects. The `value` should be
		`+ a negative number for downvoting or a positive number for upvoting.`
		`+ """`
		`+ score = models.CoprScore(copr_id=copr.id, user_id=flask.g.user.id,`
		`+ score=(1 if value > 0 else -1))`
		`+ db.session.add(score)`
		`+ return score`
		`+`
		`+ @classmethod`
		`+ def reset(cls, copr):`
		`+ cls.get(copr, flask.g.user).delete()`
		`+`
		`+`
		`class MockChrootsLogic(object):`
		`@classmethod`
		`def get(cls, os_release, os_version, arch, active_only=False, noarch=False):`

frontend/coprs_frontend/coprs/models.py

file modified

+49

		`@@ -198,6 +198,18 @@`
		`except IOError:`
		`return ""`

		`+ def score_for_copr(self, copr):`
		`+ """`
		+ Check if the `user` has voted for this `copr` and return 1 if it was
		+ upvoted, -1 if it was downvoted and 0 if the `user` haven't voted for
		`+ it yet.`
		`+ """`
		`+ query = db.session.query(CoprScore)`
		`+ query = query.filter(CoprScore.copr_id == copr.id)`
		`+ query = query.filter(CoprScore.user_id == self.id)`
		`+ score = query.first()`
		`+ return score.score if score else 0`
		`+`

		`class PinnedCoprs(db.Model, helpers.Serializer):`
		`"""`
		`@@ -215,6 +227,24 @@`
		`group = db.relationship("Group")`


		`+ class CoprScore(db.Model, helpers.Serializer):`
		`+ """`
		`+ Users can upvote or downvote projects`
		`+ """`
		`+ id = db.Column(db.Integer, primary_key=True)`
		`+ copr_id = db.Column(db.Integer, db.ForeignKey("copr.id"), nullable=False, index=True)`
		`+ user_id = db.Column(db.Integer, db.ForeignKey("user.id"), nullable=False)`
		`+ score = db.Column(db.Integer, nullable=False)`
		`+`
		`+ copr = db.relationship("Copr")`
		`+ user = db.relationship("User")`
		`+`
		`+ __table_args__ = (`
		`+ db.UniqueConstraint("copr_id", "user_id",`
		`+ name="copr_score_copr_id_user_id_uniq"),`
		`+ )`
		`+`
		`+`
		`class _CoprPublic(db.Model, helpers.Serializer, CoprSearchRelatedData):`
		`"""`
		`Represents public part of a single copr (personal repo with builds, mock`
		`@@ -567,6 +597,25 @@`

		`return list(dependencies)`

		`+ @property`
		`+ def votes(self):`
		`+ query = db.session.query(CoprScore)`
		`+ query = query.filter(CoprScore.copr_id == self.id)`
		`+ return query`
		`+`
		`+ @property`
		`+ def upvotes(self):`
		`+ return self.votes.filter(CoprScore.score == 1).count()`
		`+`
		`+ @property`
		`+ def downvotes(self):`
		`+ return self.votes.filter(CoprScore.score == -1).count()`
		`+`
		`+ @property`
		`+ def score(self):`
		`+ return sum([self.upvotes, self.downvotes * -1])`
		`+`
		`+`
		`class CoprPermission(db.Model, helpers.Serializer):`
		`"""`
		`Association class for Copr<->Permission relation`

frontend/coprs_frontend/coprs/static/copr.css

file modified

		`@@ -266,3 +266,11 @@`
		`.highlight .vi { color: #19177C } /* Name.Variable.Instance */`
		`.highlight .vm { color: #19177C } /* Name.Variable.Magic */`
		`.highlight .il { color: #666666 } /* Literal.Number.Integer.Long */`
		`+`
		`+ #project-score button {`
		`+ background: none !important;`
		`+ }`
		`+`
		`+ .text-green {`
		`+ color: #00A000;`
		`+ }`

frontend/coprs_frontend/coprs/static/css/custom-styles.css

file modified

+11 -1

		`@@ -47,4 +47,14 @@`
		`border-left: 1px #d1d1d1 solid;`
		`border-right: 1px #d1d1d1 solid;`
		`border-bottom: 1px #d1d1d1 solid;`
		`- }`
		`+ }`
		`+`
		`+ #project-score button {`
		`+ border: none;`
		`+ }`
		`+`
		`+ #project-score {`
		`+ text-align: center;`
		`+ margin-top: 10px;`
		`+ margin-right: 20px;`
		`+ }`

frontend/coprs_frontend/coprs/templates/_helpers.html

file modified

+41

		`@@ -754,3 +754,44 @@`
		`</span>`
		`</a>`
		`{% endmacro %}`
		`+`
		`+ {% macro render_project_voting(copr) %}`
		`+ {% set score = g.user.score_for_copr(copr) if g.user else 0 %}`
		`+`
		`+ {% set score_color = 'text-default' %}`
		`+ {% if score > 0 %}`
		`+ {% set score_color = 'text-green' %}`
		`+ {% elif score < 0 %}`
		`+ {% set score_color = 'text-danger' %}`
		`+ {% endif %}`
		`+`
		`+ <div id="project-score" class="pull-left">`
		`+ <form action="{{ copr_url('coprs_ns.copr_detail', copr) }}" method="POST">`
		`+ <ul class="nav nav-pills nav-stacked">`
		`+ <li role="presentation" class="{% if score > 0 %}{{ score_color }}{% endif %}">`
		`+ {% if score > 0 %}`
		`+ <button name="reset" type="submit" value="reset">`
		`+ {% else %}`
		`+ <button name="upvote" type="submit" value="upvote">`
		`+ {% endif %}`
		`+ <i class="fa fa-chevron-up fa-lg"></i>`
		`+ </button>`
		`+ </li>`
		`+ <li role="presentation">`
		`+ <span class="text-lg {{ score_color }}" title="{{ copr.upvotes }} upvotes and {{ copr.downvotes }} downvotes">`
		`+ {{ copr.score }}`
		`+ </span>`
		`+ </li>`
		`+ <li role="presentation" class="{% if score < 0 %}{{ score_color }}{% endif %}">`
		`+ {% if score < 0 %}`
		`+ <button name="reset" type="submit" value="reset">`
		`+ {% else %}`
		`+ <button name="downvote" type="submit" value="downvote">`
		`+ {% endif %}`
		`+ <i class="fa fa-chevron-down fa-lg"></i>`
		`+ </button>`
		`+ </li>`
		`+ </ul>`
		`+ </form>`
		`+ </div>`
		`+ {% endmacro %}`

frontend/coprs_frontend/coprs/templates/coprs/detail.html

file modified

+24 -19

		`@@ -1,5 +1,5 @@`
		`{% extends "layout.html" %}`
		`- {% from "_helpers.html" import copr_title, copr_url, render_crumb, copr_name %}`
		`+ {% from "_helpers.html" import copr_title, copr_url, render_crumb, copr_name, render_project_voting %}`

		`{% block title %}{{ copr_name(copr) }} Copr{% endblock %}`
		`{% block main_menu_projects %}active{% endblock %}`
		`@@ -26,27 +26,32 @@`
		`{{ alert('Forked project is still being prepared for you. It may take a few minutes to duplicate backend data.', 'warning') }}`
		`{% endif %}`

		`- <!-- PROJECT NAME -->`
		`- <div style="margin-bottom:22px;margin-top:22px">`
		`- <h1 class="project-name" style="display:inline;">{{ copr_title(copr) }}</h1>`
		`- {% if copr.delete_after %}`
		`- <small><i>temporary project: {{ copr.delete_after_msg }}</i></small>`
		`- {% endif %}`
		`- {% if copr.forked_from %}`
		`- <small><i>( forked from {{ copr_title(copr.forked_from) }})</i></small>`
		`- {% endif %}`
		`- <p class="text-muted">Project ID: {{ copr.id }}</p>`
		`- </div>`

		`+ <div class="clearfix">`
		`+ <!-- Upvote or downvote the project -->`
		`+ {{ render_project_voting(copr) }}`
		`+`
		`+ <!-- PROJECT NAME -->`
		`+ <div style="margin-bottom:22px;margin-top:22px">`
		`+ <h1 class="project-name" style="display:inline;">{{ copr_title(copr) }}</h1>`
		`+ {% if copr.delete_after %}`
		`+ <small><i>temporary project: {{ copr.delete_after_msg }}</i></small>`
		`+ {% endif %}`
		`+ {% if copr.forked_from %}`
		`+ <small><i>( forked from {{ copr_title(copr.forked_from) }})</i></small>`
		`+ {% endif %}`
		`+ <p class="text-muted">Project ID: {{ copr.id }}</p>`
		`+ </div>`

		`- {% macro nav_element(tab_name, tab_title, icon, href) %}`
		`- <li class="{% if selected_tab == tab_name %}active{% endif %}">`
		`- <a href="{{ href }}"> <span class="{{icon}}"></span>{% if "pficon" not in icon %} {%endif%}`
		`- {{ tab_title }}`
		`- </a>`
		`- </li>`
		`- {% endmacro %}`

		`+ {% macro nav_element(tab_name, tab_title, icon, href) %}`
		`+ <li class="{% if selected_tab == tab_name %}active{% endif %}">`
		`+ <a href="{{ href }}"> <span class="{{icon}}"></span>{% if "pficon" not in icon %} {%endif%}`
		`+ {{ tab_title }}`
		`+ </a>`
		`+ </li>`
		`+ {% endmacro %}`
		`+ </div>`


		`<!-- MENU -->`

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py

file modified

+30

		`@@ -331,6 +331,36 @@`
		`)`


		`+ @coprs_ns.route("/<username>/<coprname>/", methods=["POST"])`
		`+ @coprs_ns.route("/g/<group_name>/<coprname>/", methods=["POST"])`
		`+ @req_with_copr`
		`+ @login_required`
		`+ def copr_detail_post(copr):`
		`+ form = forms.VoteForCopr(meta={'csrf': False})`
		`+ if not form.validate_on_submit():`
		`+ flask.flash(form.errors, "error")`
		`+ return render_copr_detail(copr)`
		`+`
		`+ # Always reset the current vote`
		`+ coprs_logic.CoprScoreLogic.reset(copr)`
		`+`
		`+ if form.upvote.data:`
		`+ coprs_logic.CoprScoreLogic.upvote(copr)`
		`+ if form.downvote.data:`
		`+ coprs_logic.CoprScoreLogic.downvote(copr)`
		`+ db.session.commit()`
		`+`
		`+ # Return to the previous site. The vote could be sent from`
		`+ # packages/builds/settings/etc page, so we don't want to just`
		+ # `render_copr_detail` but return to the previous page instead
		`+ if flask.request.referrer:`
		`+ return flask.redirect(flask.request.referrer)`
		`+`
		`+ # HTTP referrer is unreliable so as a fallback option,`
		`+ # we just render the project overview page`
		`+ return flask.redirect(helpers.copr_url("coprs_ns.copr_detail", copr))`
		`+`
		`+`
		`@coprs_ns.route("/<username>/<coprname>/permissions/")`
		`@coprs_ns.route("/g/<group_name>/<coprname>/permissions/")`
		`@req_with_copr`

frontend/coprs_frontend/tests/test_logic/test_coprs_logic.py

file modified

+23 -1

		`@@ -12,7 +12,8 @@`
		`from coprs import app`
		`from coprs.forms import PinnedCoprsForm, ChrootForm, ModuleEnableNameValidator`
		`from coprs.logic.actions_logic import ActionsLogic`
		`- from coprs.logic.coprs_logic import CoprsLogic, CoprChrootsLogic, PinnedCoprsLogic`
		`+ from coprs.logic.coprs_logic import (CoprsLogic, CoprChrootsLogic,`
		`+ PinnedCoprsLogic, CoprScoreLogic)`
		`from coprs.logic.users_logic import UsersLogic`
		`from coprs.logic.complex_logic import ComplexLogic`

		`@@ -326,3 +327,24 @@`

		`form.module_toggle.data = "module: stream"`
		`assert False == form.validate()`
		`+`
		`+`
		`+ class TestCoprScoreLogic(CoprsTestCase):`
		`+`
		`+ @new_app_context`
		`+ @pytest.mark.usefixtures("f_users", "f_coprs", "f_db")`
		`+ def test_score_math(self):`
		`+ assert self.c1.score == 0`
		`+`
		`+ flask.g.user = self.u1`
		`+ CoprScoreLogic.upvote(self.c1)`
		`+`
		`+ flask.g.user = self.u2`
		`+ CoprScoreLogic.downvote(self.c1)`
		`+`
		`+ flask.g.user = self.u3`
		`+ CoprScoreLogic.downvote(self.c1)`
		`+`
		`+ assert self.c1.upvotes == 1`
		`+ assert self.c1.downvotes == 2`
		`+ assert self.c1.score == -1`

frostyx commented 3 years ago

This change will show a reddit-style upvote and downvote
buttons next to the project name. It is implemented in a way
that we can see the sum of a project score as one number but
we can also see the number of upvotes and downvotes separately
because even though the overall score of a project might be high,
if there is considerable amount of downvotes, there might be
something fishy about it.

The score and voting is available to be seen by an anonymous
user, however cliking on a upvote/downvote button will redirect
him to the login page.

praiskup commented 3 years ago

Can you please rebase on top of master? The build failure is weird.

rebased onto f6681feb8042bf8fdb120f072461f98f112ff72f

3 years ago

frostyx commented 3 years ago

Hmm, the build failure actually points out to a bug that I overlooked. Gonna fix that.

rebased onto 894e08a1791afce8c4523700e7e4d0217d1edbb5

3 years ago

frostyx commented 3 years ago

To elaborate - The bug was that voting properly showed only for the overview page, anything else failed with 500. Hence tests started failing because it couldn't get output for subpages.

It is fixed now and the build is passing. PTAL.

rebased onto e4524a2f15c5c46d49a010d829d55034e24d2500

3 years ago

frostyx commented 3 years ago

I did one small rebase fixing some meaningful pylint warnings.

praiskup commented 3 years ago

It is important to see if I already voted or not - but I don't see any difference in the arrows:

Can we make the background color transparent (normally), and make it darker when I already voted?

praiskup commented on line 15 of frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py 3 years ago

This should probably be the top-most command, even before form variable is instantiated?

praiskup commented 3 years ago

Otherwise looks very nice!

frostyx commented 3 years ago

It is important to see if I already voted or not - but I don't see any difference in the arrows:

Normally, both arrows are gray. The green arrow signalizes, that you already upvoted the project. Do you think it should be more clear? (I can say this how many times I want, it always sounds as sarcasm in my head. It is meant to be a serious question)

Looking at reddit voting again, they also change the color of the score number so it is more obvious.

Can we make the background color transparent (normally), and make it darker when I already voted?

Backgroud color of the whole voting area? Or just around that one arrow? I have hard time imagining that.

frostyx commented on line 15 of frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py 3 years ago

That's a good catch. I agree. And I think we should even have a decorator for that, let me fix that.

dturecek commented 3 years ago

I agree it's hard to see whether I've voted or no - I see the difference but not on the first glance. I think the contrast between the two arrows should be higher, otherwise I tend to click on the arrows again just to be sure.

praiskup commented 3 years ago

I think the contrast between the two arrows should be higher

Indeed. I now noticed it is actually green :-) but at first glance it is not obvious..

Normally, both arrows are gray.

Can you make the arrow background transparent? I think it would look much better.

rebased onto 46b55934d1d456726181aceb1fd7373db261e319

3 years ago

frostyx commented 3 years ago

Ok ok, thank you for the feedback guys.

The green is inherited from text-success. I agree that it could be a bit darker but I didn't want to introduce a custom color. Anyway, I have now colored even the score number, so I think it looks way better now. Check this out.

Can you make the arrow background transparent? I think it would look much better.

I am going to try that

That's a good catch. I agree. And I think we should even have a decorator for that, let me fix that.

Fixed

I also noticed that a positioning of that voting box got broken when you tried to log in or out, so I also fixed that.

Edited 3 years ago by frostyx

praiskup commented 3 years ago

Anyway, I have now colored even the score number, so I think it looks way better now.

On my not-so-hi-end IPS display (T580) I am not really able to recognize that I voted UP.
The green color is simply too similar to the grey variant. I don't actually think we need darker but lighter green color.

The red-variant for downvote is good, though.

frostyx commented 3 years ago

Also, what is your opinion on this topic - should a project automatically get an upvote from its creator?

praiskup commented 3 years ago

should a project automatically get an upvote from its creator

That would be weird, is this implemented somewhere? I think we could disallow people to upvote/downvote their projects - but even there I don't see a motivation TBH.

praiskup commented on line 27 of frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py 3 years ago

I'm not sure whether this doesn't throw traceback if HTTP_REFERER is undefined.

rebased onto 9d1ddc823ad9b188deed7af3ba0e6ab1c29f0667

3 years ago

frostyx commented 3 years ago

I'm not sure whether this doesn't throw traceback if HTTP_REFERER is undefined.

I used the project overview page as a fallback and also used flask.request.referrer accessing the header directly. It doesn't look so weird now because we already have some instance of that in the code.

I also fixed the weird background color around the voting arrows. I haven't seen them before, but once I lovered my monitor contrast setting, they appeared.

I switched the green from text-success to #272 which is the green color that we use for e.g. printing "succeeded" or "forked" build status. Do you see it clearer than the original one? Screenshot here - Greener upvote

The only thing missing is a unit test.

praiskup commented on line 32 of frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py 3 years ago

I'd be slightly more confident if we redirected here as well, but It's not a huge thing ....

praiskup commented 3 years ago

I also fixed the weird background color around the voting arrows. I haven't seen them before, but once I lovered my monitor contrast setting, they appeared.

Looks good.

I switched the green from text-success to #272 which is the green color that we use for e.g. printing "succeeded" or "forked" build status. Do you see it clearer than the original one?

Unfortunately, it's only a bit better. But still, I need to focus a lot -- it's not obvious.

rebased onto c16c711984ebf2e6fb44f7be3433ae907aa7cab7

3 years ago

frostyx commented 3 years ago

Unfortunately, it's only a bit better. But still, I need to focus a lot -- it's not obvious.

Okay, what about we ditch green color altogether. Reddit also uses orange color for posts that you upvoted, what if we try that?

Orange upvotes

I'd be slightly more confident if we redirected here as well, but It's not a huge thing ....

Fixed

The only thing missing is a unit test.

I added one test that made sense to me. If you would like to test some other use-case, please let me know.

praiskup commented 3 years ago

The orange color is nicely visible. Even though I think that the green is slightly more matching the purpose. What about limegreen? :-) Or keep both up/down highlight color the same ~~e.g.#275ba4~~ (edit: the blue default copr color is not visible, either)?

Edited 3 years ago by praiskup

praiskup commented 3 years ago

gray/lightgray would be nice as well :-)

rebased onto 9133dad3d8b0a2d4316a00745c06fd13b345064c

3 years ago

frostyx commented 3 years ago

We discussed this off-list so we can speed up the discussion a little and I think we can settle on this one https://frostyx.fedorapeople.org/pagure/copr-upvote-limeish.png

praiskup commented 3 years ago

This is not true statement anymore:

    If we decide that the button is so small, that no shade of green
    would be distinctable enough, we can switch to orange button for
    both upvote and downvote.

rebased onto ff1ec5815a636ea617cff2513fd739830ed59279

3 years ago

frostyx commented 3 years ago