Skip to content

GitLab

Merged
Created by Administrator@rootOwner

Allow public repo searching

Created by: karlhungus

This PR fixes this issue: https://github.com/gitlabhq/gitlabhq/issues/5349

Loading
Loading
  • Administrator
    Administrator @root started a thread on commit b89d8497
    app/controllers/search_controller.rb
    23 14 @blobs = Kaminari.paginate_array(result[:blobs]).page(params[:page]).per(20)
    24 15 @total_results = @projects.count + @merge_requests.count + @issues.count + @wiki_pages.count + @blobs.total_count
    25 16 end
    17
    18 private
    19
    20 def find_project_ids(group_id, project_id)
    21 project_ids = current_user.authorized_projects.map(&:id)
    22
    23 if group_id.present?
    24 @group = Group.find(group_id)
    25 group_project_ids = @group.projects.map(&:id)
    26 project_ids.select! { |id| group_project_ids.include?(id) }
    27 elsif project_id.present?
    • Administrator
      Administrator @root ·
      Owner

      Created by: karlhungus

      this is essentially the only change. extracted a method to make testing easier

      By Administrator on 2013-11-13T20:08:20 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit b89d8497
    app/controllers/search_controller.rb
    23 14 @blobs = Kaminari.paginate_array(result[:blobs]).page(params[:page]).per(20)
    24 15 @total_results = @projects.count + @merge_requests.count + @issues.count + @wiki_pages.count + @blobs.total_count
    25 16 end
    17
    18 private
    19
    20 def find_project_ids(group_id, project_id)
    21 project_ids = current_user.authorized_projects.map(&:id)
    22
    23 if group_id.present?
    24 @group = Group.find(group_id)
    25 group_project_ids = @group.projects.map(&:id)
    26 project_ids.select! { |id| group_project_ids.include?(id) }
    27 elsif project_id.present?
    • Administrator
      Administrator @root ·
      Owner

      Created by: jvanbaarsen

      Maybe this method can be private? Or is it essential it is part of the public API?

      By Administrator on 2013-11-13T20:08:20 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit b89d8497
    app/controllers/search_controller.rb
    23 14 @blobs = Kaminari.paginate_array(result[:blobs]).page(params[:page]).per(20)
    24 15 @total_results = @projects.count + @merge_requests.count + @issues.count + @wiki_pages.count + @blobs.total_count
    25 16 end
    17
    18 private
    19
    20 def find_project_ids(group_id, project_id)
    21 project_ids = current_user.authorized_projects.map(&:id)
    22
    23 if group_id.present?
    24 @group = Group.find(group_id)
    25 group_project_ids = @group.projects.map(&:id)
    26 project_ids.select! { |id| group_project_ids.include?(id) }
    27 elsif project_id.present?
    • Administrator
      Administrator @root ·
      Owner

      Created by: karlhungus

      there is no reason it has to be public, it makes testing slightly more of a pain

      By Administrator on 2013-11-13T20:08:20 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit b89d8497
    app/controllers/search_controller.rb
    23 14 @blobs = Kaminari.paginate_array(result[:blobs]).page(params[:page]).per(20)
    24 15 @total_results = @projects.count + @merge_requests.count + @issues.count + @wiki_pages.count + @blobs.total_count
    25 16 end
    17
    18 private
    19
    20 def find_project_ids(group_id, project_id)
    21 project_ids = current_user.authorized_projects.map(&:id)
    22
    23 if group_id.present?
    24 @group = Group.find(group_id)
    25 group_project_ids = @group.projects.map(&:id)
    26 project_ids.select! { |id| group_project_ids.include?(id) }
    27 elsif project_id.present?
    • Administrator
      Administrator @root ·
      Owner

      Created by: jvanbaarsen

      Maybe you can make it private? (So it dont need any special testing)

      By Administrator on 2013-11-13T20:08:20 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit b89d8497
    app/controllers/search_controller.rb
    23 14 @blobs = Kaminari.paginate_array(result[:blobs]).page(params[:page]).per(20)
    24 15 @total_results = @projects.count + @merge_requests.count + @issues.count + @wiki_pages.count + @blobs.total_count
    25 16 end
    17
    18 private
    19
    20 def find_project_ids(group_id, project_id)
    21 project_ids = current_user.authorized_projects.map(&:id)
    22
    23 if group_id.present?
    24 @group = Group.find(group_id)
    25 group_project_ids = @group.projects.map(&:id)
    26 project_ids.select! { |id| group_project_ids.include?(id) }
    27 elsif project_id.present?
    • Administrator
      Administrator @root ·
      Owner

      Created by: karlhungus

      heh, makes testing more of a pain to make it private. I have got to resetup my environment since i had to give my old laptop back but i`ll get to it when i can.

      By Administrator on 2013-11-13T20:08:20 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jsternberg

    This may also be fixed by PR #5562 which is more generic than this. Can you see if that also fixes your problem? I think the solution is better than adding a new function for just one location where this was a problem.

    By Administrator on 2013-11-12T18:38:47 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jsternberg just getting an environment setup, i'll see, one way to check yourself would be to take the unit test i provided and run it against the new stuff. then you get the benefit of additional coverage and know this issue has been addressed. another (crappier) way would be to just test it by hand.

    By Administrator on 2013-11-13T19:22:12 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jsternberg allright i verified that the tests still fail, pretty sure this would cause crapout on the context, but i didn't verify that (i.e. i didn't verify that https://github.com/gitlabhq/gitlabhq/issues/5349 is fixed in the app -- although my from brief look at the search_context this is still a problem).

    @jvanbaarsen allright the method is no private.

    Also rebased on master

    By Administrator on 2013-11-13T20:10:48 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jsternberg

    @karlhungus of course the test is going to fail. It calls a function that you added and I didn't. Please verify that #5349 (closed) is fixed, not that your unit test works.

    By Administrator on 2013-11-13T20:23:01 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jsternberg

    Also, your issue seems to have already been fixed with a27e4404.

    By Administrator on 2013-11-13T20:28:32 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jsternberg I'm going to assume your comment just sounds insulting, and wasn't intended that way (it does sound that way, so you might consider in the future this isn't a fantastic way to get things done).

    I've verified the code as it exists on the tip of master, and that my method extraction is identical with the exception of the change mentioned above in "this is essentially the only change. extracted a method to make testing easier".

    https://github.com/gitlabhq/gitlabhq/commit/a27e440 Is actually a fix i made here https://github.com/gitlabhq/gitlabhq/pull/5435, that allows searching other aspects of public repos (name/namespace).

    The problem this fixes is that the public repo won't be set as the @project, when the code search happens in search_context.rb, this (https://github.com/gitlabhq/gitlabhq/issues/5349) will still be an issue.

    edit: clarification

    By Administrator on 2013-11-13T20:49:30 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jsternberg

    Sorry, I thought you ran your test against my code verbatim, saw an error, then responded with "it doesn't work." I thought it was quite rude myself.

    From the looks of things, my pull request should fix that. Since the list of project_ids would be populated from all projects, including public ones, it will include the specific project_id and the list of project_ids should be filtered correctly. You should be able to search a public project. It would also remove the need to have SearchContext include public projects, since the public projects would be included in the project_ids passed to SearchContext to begin with. So I think it would invalidate the need for the previous pull request you made.

    The core problem (which is what my pull request addresses) is that the list of projects does not include public projects in certain situations related to permissions.

    Have you tried out my fix to see if it fixes your 500 error? That's all I was asking, as I think it's a better solution to the problem.

    By Administrator on 2013-11-13T21:35:30 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jsternberg my apologies, i assumed your PR was merged already.

    Looking at overlaps specifically, this change

    -    project_ids = current_user.authorized_projects.map(&:id)

+    project_ids = current_user.all_projects.map(&:id)

    Yeah, your solution I would expect to also fix this (@project not getting set and search_context.rb exploding). You might have a bit of a harder time getting it accepted (obviously not anything i have anything to do with) on account of the all_projects query possibly returning lots of results (with full project object graphs populated), maybe a pluck is the right thing to do here.

    By Administrator on 2013-11-13T21:51:18 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jsternberg FWIW i also prefer your solution (although i think the method extraction adds readability).

    By Administrator on 2013-11-14T01:52:43 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jsternberg Just to elaborate a bit (had to put kids to bed last night so couldn't give you a good response).

    Insults: my point of view is you said this may fix your problem, i think my suggestion (taking my unit test, and obviously rewriting it, or even better doing the same method extraction) would have born this out. Alternatively actually testing it. The point here is that you suggested your fix may fix the problem i'm trying to solve, which would have been easier for you to prove out then me. I came to see this as you coming in and getting me to do all the work of verification (which i've already done on my own PR -- it's time consuming and a pain in the ass).

    Code quality: your solution is the obviously superiorsolution, it is more general and addresses more bugs. My fix is just that, only to fix this bug.

    As i said previously, I would recommend you look at the queries you are going to generate getting all projects (as their object graphs) a user can see as not necessarily scaling well (I'm actually not sure what rails will do here -- if it will optimize it to essentially only getting id's or if it will try to fill out the whole object).

    By Administrator on 2013-11-14T15:47:00 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jsternberg

    The first time I posted I hadn't tested your problem against mine, but the problems seemed similar and that it would be caused by the same problem, which is the reason I posted to begin with. I had already tested my pull request on my issue and verified it. I thought verifying it for you would just be "fetch, checkout, bundle install, bundle exec" and then trying to do the same search. I hadn't realized you destroyed your testing environment.

    If we want to continue discussing the ramifications of my solution (which would be potentially loading all public projects), I think we should do it in my pull request. I need to double check the speed. For my own problem, it only retrieved the ids so there was no extra loading from the database unless another section of code goes and loads those project ids afterwards (which from the looks of it, it does at some point). I debated when I was doing this if searching should be the authorized_projects list or the all_projects list. Unfortunately, even if I am loading the public projects, the reason why I did that to begin with was because I did need the search box to include public projects.

    By Administrator on 2013-11-14T16:17:00 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jvanbaarsen

    This pull request has been closed because a request for more information has not been reacted to for more than 2 weeks. If you respond and conform to the pull request guidelines in our contributing guidelines we will reopen this pull request. /cc @Razer6

    By Administrator on 2013-12-10T17:30:06 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: karlhungus

    @jvanbaarsen not sure what information you've requested, but go ahead and close it if you need to

    By Administrator on 2013-12-10T17:35:22 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jvanbaarsen

    @jsternberg posted a message. If you want this to be merged no problem :-) We will leave this PR open 😄 @randx can you take a look?

    By Administrator on 2013-12-10T17:37:12 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jvanbaarsen

    @randx What do you think about this functionality? /cc @dosire ]

    By Administrator on 2014-01-05T12:43:01 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    thanks

    By Administrator on 2014-01-09T08:24:56 (imported from GitLab project)

Please register or sign in to reply