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

Add project stars. #7233

Merged
merged 7 commits into from Jul 24, 2014

Conversation

7 participants
@cirosantilli
Contributor

cirosantilli commented Jul 1, 2014

Fix ACCEPTING MR at http://feedback.gitlab.com/forums/176466-general/suggestions/4518904-star-a-project-to-show-it-at-the-top-of-your-dashb points 1. and 2.

Before hover:

screenshot from 2014-07-01 12 40 55 no hover

On hover:

screenshot from 2014-07-01 12 41 20 hover

After click:

screenshot from 2014-07-01 12 41 53 after click

Signed out:

screenshot from 2014-07-01 12 43 23 signed out

Implementation notes

Ignoring "Use nested module/class definitions instead of compact style." Hound warning as that style is used by all the rest of the API and recommended by Spinach itself. I propose we turn that warning off if possible.

@TeatroIO

This comment has been minimized.

TeatroIO commented Jul 1, 2014

I've prepared a stage. Click to open.

@@ -507,4 +509,17 @@ def log_info message
def system_hook_service
SystemHooksService.new
end
def starred? project

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Use def with parentheses when there are parameters.

starred_projects.exists?(project)
end
def toggle_star project

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Use def with parentheses when there are parameters.

validates :user, presence: true
validates :user_id, uniqueness: {
scope: [:project_id],
message: 'already starred'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# TODO remove this: add it t SharedAuth
include SharedUser
step 'The project has 0 stars' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

has_n_stars(0)
end
step 'The project has 1 star' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

has_n_stars(1)
end
step 'The project has 2 stars' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

end
# Requires @javascript
step 'I click on the star toggle button' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# Requires @javascript
step 'I click on the star toggle button' do
page.find('.star .toggle', visible: true).click

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

protected
def has_n_stars(n)
expect(page).to have_css('.star .count', text: /^#{n}$/, visible: true)

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -24,6 +24,10 @@ module SharedAuthentication
current_path.should == new_user_session_path
end
step 'I logout' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# ----------------------------------------
step 'I visit project "Community" page' do
project = Project.find_by(name: 'Community')

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

end
step 'I visit project "Internal" page' do
project = Project.find_by(name: 'Internal')

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

end
step 'I visit project "Enterprise" page' do
project = Project.find_by(name: 'Enterprise')

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# Empty Projects
# ----------------------------------------
step 'I visit empty project page' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# ----------------------------------------
step 'I visit empty project page' do
project = Project.find_by(name: 'Empty Public Project')

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# ----------------------------------------
step 'public empty project "Empty Public Project"' do
create :empty_project, :public, name: 'Empty Public Project'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -9,6 +9,11 @@ module SharedUser
user_exists("Mary Jane", {username: "mary_jane"})
end
step 'I delete my account' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -9,6 +9,11 @@ module SharedUser
user_exists("Mary Jane", {username: "mary_jane"})
end
step 'I delete my account' do
visit profile_account_path
click_link 'Delete account'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

let(:project) { create(:project) }
let(:public_project) { create(:project, :public) }
let(:user) { create(:user) }
let(:jpg) { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Line is too long. [106/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

let(:public_project) { create(:project, :public) }
let(:user) { create(:user) }
let(:jpg) { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
let(:txt) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Line is too long. [105/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -40,4 +41,17 @@
end
end
end
describe "POST #toggle_star" do
it 'increases star count if user is signed in' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

expect(public_project.star_count).to eq(1)
end
it 'does nothing if user is not signed in' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -20,6 +20,6 @@ def login_with(user)
end
def logout
click_link "Logout" rescue nil
page.find(:css, '.icon-signout').click rescue nil

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

# ----------------------------------------
step 'public empty project "Empty Public Project"' do
create :empty_project, :public, name: 'Empty Public Project'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -9,6 +9,11 @@ module SharedUser
user_exists("Mary Jane", {username: "mary_jane"})
end
step 'I delete my account' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -9,6 +9,11 @@ module SharedUser
user_exists("Mary Jane", {username: "mary_jane"})
end
step 'I delete my account' do
visit profile_account_path
click_link 'Delete account'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

let(:project) { create(:project) }
let(:public_project) { create(:project, :public) }
let(:user) { create(:user) }
let(:jpg) { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Line is too long. [106/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

let(:public_project) { create(:project, :public) }
let(:user) { create(:user) }
let(:jpg) { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
let(:txt) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Line is too long. [105/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -40,4 +41,17 @@
end
end
end
describe "POST #toggle_star" do
it 'increases star count if user is signed in' do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -262,7 +262,7 @@
issue.save
end
it 'shows milestone text' do
it 'shows milestone text', js: true do

This comment has been minimized.

@houndci-bot

houndci-bot Jul 4, 2014

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 4, 2014

Fixed RSpec failures which I caused due to driver incompatibilities... for some reason click_link "Logout" failed on @javascript on the feature and find(:css, 'icon').click fails without js: true on the RSpec... I don't know why. It works on both as it is now.

https://travis-ci.org/gitlabhq/gitlabhq/jobs/29121023#L853 passed locally and is unrelated to this changed

https://travis-ci.org/gitlabhq/gitlabhq/jobs/29121026#L1182 happens on this PR, but passes locally. I don't understand why it is failing on Travis.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 12, 2014

Can I add a count column to projects that keeps the running count stored? This would speed up the single count query here, and would be fundamental for http://feedback.gitlab.com/forums/176466-general/suggestions/5647999-show-popular-project-on-the-public-projects-page where we would need the star count of all projects at once.

@jvanbaarsen

This comment has been minimized.

Contributor

jvanbaarsen commented Jul 12, 2014

@cirosantilli You have my vote for adding the count to the database, another option would be to store it in redis.
@randx What do you think?

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 12, 2014

I know very little about Redis, IIUC the single advantage of using it in this case is greater speed? If so, we might stick the new RDMS column for simplicity, and optimize later if it becomes the speed bottleneck.

@jvanbaarsen

This comment has been minimized.

Contributor

jvanbaarsen commented Jul 12, 2014

@cirosantilli Lets wait for @randx's opinion on this :)

$('.star').on 'ajax:success', (e, data, status, xhr) ->
$(@).toggleClass('on').find('.count').html(data.star_count)
.on 'ajax:error', (e, xhr, status, error) ->
new Flash('Star toggle failed. Try again later.', 'alert')

This comment has been minimized.

@dzaporozhets

dzaporozhets Jul 14, 2014

Member

move javascript to project.js.coffee file if possible

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Jul 14, 2014

@cirosantilli At first I thought db index will be enough. But after looking at http://feedback.gitlab.com/forums/176466-general/suggestions/5647999-show-popular-project-on-the-public-projects-page I agree it makes sense to add stars field to project table. It will make most starred projects query much easier. So implement at will.

Also please make this PR mergeable. When ready to merge - please assign it to me. I will do review and merge if ok

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Jul 14, 2014

Also tests should be green :)

@@ -60,6 +60,8 @@ def show
@events = event_filter.apply_filter(@events)
@events = @events.limit(limit).offset(params[:offset] || 0)
@show_star = !(current_user and current_user.starred?(@project))

This comment has been minimized.

@houndci-bot

houndci-bot Jul 15, 2014

Use && instead of and.

@@ -60,6 +60,8 @@ def show
@events = event_filter.apply_filter(@events)
@events = @events.limit(limit).offset(params[:offset] || 0)
@show_star = !(current_user and current_user.starred?(@project))

This comment has been minimized.

@houndci-bot

houndci-bot Jul 15, 2014

Use && instead of and.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 15, 2014

All tests pass locally.

RSpec failures seem to be unrelated: https://travis-ci.org/gitlabhq/gitlabhq/jobs/29964656, and also happen on other merge requests like https://travis-ci.org/gitlabhq/gitlabhq/jobs/29956956

The Spinach failure https://travis-ci.org/gitlabhq/gitlabhq/jobs/29964450#L3085 was introduced by this and is consistent on Travis, but passes locally consistently. I am unable to understand why: it should be a simple click by anchor content. I have already searched a lot, and cannot understand why. If no one can explain it, I'll just remove that test (and move it to the model specs for instance).

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Jul 17, 2014

@cirosantilli can you make it mergeable?

= render 'link_to_toggle_star',
title: 'You must sign in to star a project.',
starred: false,
signed_in: false

This comment has been minimized.

@dzaporozhets

dzaporozhets Jul 17, 2014

Member

@cirosantilli can you move this into helper instead template? Lets avoid logic in views

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 17, 2014

@randx I'll fix those points. What shall be done about the Spinach failure?

@jvanbaarsen

This comment has been minimized.

Contributor

jvanbaarsen commented Jul 17, 2014

@cirosantilli I think they need to be fixed up, since at this point they are failing on what looks like part of this feature.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 17, 2014

@jvanbaarsen agree. As mentioned in #7233 (comment), they pass locally, and I have searched a lot and I don't understand why they fail on Travis. If no one has a clue I'll remove the failing feature test and put it on the model. Still, I'd like to understand.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jul 18, 2014

@randx should be OK now:

  • removed the failing feature and rewrote as model. Probably some Js engine incompatibility... don't know.
  • current errors are unrelated: all PgSQL pass. The MySQL is deadlocking sometimes, might be a bug in GitLab only for MySQL.
@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Jul 24, 2014

@cirosantilli cool. I will try it

@dzaporozhets dzaporozhets merged commit e1d307b into gitlabhq:master Jul 24, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details
@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Jul 24, 2014

@cirosantilli thank you!

@cirosantilli cirosantilli deleted the cirosantilli:star branch Jul 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment