Redmine

Marius Thank you for confirming this issue.
I also dipped my toe into the code and the repository. I can confirm that the line you modify in your patch is the cause of that issue. You could also remove the entire line, which has the same effect as your attached conditional statement. The annotation of @queries_helper.rb reveals that this line (source:trunk/app/helpers/queries_helper.rb#L297) was introduced in r7656 to fix #9108.

Applying your patch (or removing the line, as I did) breaks the fix for #9108 and causes failing tests in issues_controller_test.rb.

I seems to me that overriding the queries project id in r7656 was a small and cheap hack to fight the symptoms but not the cause of the problem.
The actual problem of #9108 is the fact that the project id for the session is extracted from the query what doesn't work if the query is global and has no project.

So I think if we get #9108 properly fixed, the issue described here is solved, too.

Bernhard Rohloff wrote:

Applying your patch (or removing the line, as I did) breaks the fix for #9108 and causes failing tests in issues_controller_test.rb.

You're able to reproduce the issue from #9108 with the patch applied? I'm asking because I tried to follow the steps from the description and it worked as expected in both cases (global issues page and project issues page). Maybe I miss something.

I chose to add the condition instead of removing the entire line in order to change the behaviour only for global queries.

Marius BALTEANU wrote:

Bernhard Rohloff wrote:

Applying your patch (or removing the line, as I did) breaks the fix for #9108 and causes failing tests in issues_controller_test.rb.

You're able to reproduce the issue from #9108 with the patch applied? I'm asking because I tried to follow the steps from the description and it worked as expected in both cases (global issues page and project issues page). Maybe I miss something.

Oh I'm sorry! I mixed the issue numbers up... #9738 is the right one. #9108 was the issue linked with the last changeset the line was affected by. They reattached the line because they had a problem with breaking the fix of #9738, too.

I chose to add the condition instead of removing the entire line in order to change the behaviour only for global queries.

As far as I could see, the project_id is stored within the queries table in the database and the queries are selected by the id of the current project. So there is no reason for overriding it with the same id except for the query is a global one.

Bernhard Rohloff wrote:

This bug caused me quite a headache but eventually I found the root of all evil in the query model.
The problem is that the overloaded initialize method doesn't get called.
Because of this the variable @is_for_all is not properly set and returns a false state in the condition statement.

Indeed, that was my conclusion too, but I wasn't be able to find a better solution than my workaround from the previous post.

I've fixed it now with the after_initialize callback method which seems to generally be the better way to do this, in reference to http://blog.dalethatcher.com/2008/03/rails-dont-override-initialize-on.html.

Nice :) I didn't know about this callback. Today I learned something.

The attached patch is passing the tests in issues_controller_test.rb and query_controller_test.rb including the additional tests from Marius' patch.

What do you think if you add to your patch the method is_for_all?? Something like:

def is_for_all?
   @is_for_all ||= project.nil?
end

Also, it looks like that you are using tabs instead of two spaces for indentation.

Bernhard Rohloff wrote:

Yep, that sounds good. I've implemented it in my new version of the patch and also gave it a more descriptive name.

Indeed, some tabs have crept into the patch. I've fixed them.

Looks good now the patch to me.