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

Remove unused authenticate_user from project#show #8094

Merged
merged 1 commit into from Nov 5, 2014

Conversation

4 participants
@cirosantilli
Contributor

cirosantilli commented Oct 19, 2014

Redundant with the authorize_read_project! filter of the same controller.

@jhollingsworth you may have written that line. The red flag for me was the public?, which generally duplicates Ability logic.

Remove unused authenticate_user from project#show
Redundant with the authorize_read_project! filter
@TeatroIO

This comment has been minimized.

TeatroIO commented Oct 19, 2014

I've prepared a stage. Click to open.

@cirosantilli cirosantilli changed the title from [WIP] Remove unused authenticate_user from project#show to Remove unused authenticate_user from project#show Oct 19, 2014

@vsizov

This comment has been minimized.

Contributor

vsizov commented Nov 4, 2014

Thank you for the help @cirosantilli
I think we should look into this problem more deeply because duplication is here https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/application_controller.rb#L98
I'll look into. Thanks anyway :)

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Nov 4, 2014

@vsizov yes, there might be a better way to do this. In particular, my first instinct was to split up def project into two functions to decouple it because now it does two unrelated things:

  • set @project
  • check permissions
@vsizov

This comment has been minimized.

Contributor

vsizov commented Nov 4, 2014

@cirosantilli I'll let you know about result of my investigation

@vsizov

This comment has been minimized.

Contributor

vsizov commented Nov 5, 2014

I found a few places for refactoring and I will do it soon. I think we can merge it anyway. Thanks @cirosantilli

vsizov added a commit that referenced this pull request Nov 5, 2014

Merge pull request #8094 from cirosantilli/rm-unused-authenticate_user
Remove unused authenticate_user from project#show

@vsizov vsizov merged commit f9814bf into gitlabhq:master Nov 5, 2014

1 check passed

default The build passed on Semaphore.
Details

@cirosantilli cirosantilli deleted the cirosantilli:rm-unused-authenticate_user branch Nov 5, 2014

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