maxrossello / redmine_app_timesheets

A true timesheet plugin using orders, not bound to timelogs over issues
GNU General Public License v2.0
32 stars 10 forks source link

Project Roadmap crashes with PostgreSQL #69

Closed greyman888 closed 8 years ago

greyman888 commented 8 years ago

Hi Max,

Build: timesheets 1.5.5, Redmine 3.1, postgresql Config: "Shared versions visible to non members" set to nil

Bug: Accessing any project roadmap causes a crash.

The line that causes the problem is in timeheets_app_project_patch.rb:

if Setting.plugin_redmine_app_timesheets["public_versions"].nil?
    @rolled_up_versions = @rolled_up_versions.where("#{Version.table_name}.id NOT IN (?)", noperm + noallow + [""])
end

I don't think that postgreSQL likes getting directly fed nil values here.

If you change it to the line below, active record seems to take care of it automagically.

if Setting.plugin_redmine_app_timesheets["public_versions"].nil?
    @rolled_up_versions = @rolled_up_versions.where.not(id: [noperm, noallow].flatten) 
end

I must admit, I don't really understand why you have this part of the patch. If you have a little time, could let me know what it does?

I was going to raise a new pull request, but since I already have one open github wanted to combine this change with the other one.

Adam

maxrossello commented 8 years ago

Hi Adam, the whole "Shared versions visible to non members" feature is something that was nice to have when target versions needed to be shared up to the backing project in order to be treated as orders, now it makes a lot less sense. If the plugin forced people to share versions, then at least they should have enhanced privacy. I wanted to split this thing to a separate plugin, yet it needs a path for those who now rely on the feature irrespective of orders and it needs... time.

So basically those lines enforce the privacy by hiding versions that are to be hidden.

The solution you propose is ok for Redmine 3.1 but would break backward compatibility with 2.6 since the "not" statement in Active Record is available only in Rails 4

Thank you

greyman888 commented 8 years ago

Sorry about that. I'm pretty new to rails and so I don't have the history. I'll try something else.

greyman888 commented 8 years ago

Removing the [""] stops it crashing but I don't know what the consequence of doing that is. The line would then be:

@rolled_up_versions = @rolled_up_versions.where("#{Version.table_name}.id NOT IN (?)", noperm + noallow)
maxrossello commented 8 years ago

Problem with removing quotes is well described here: http://stackoverflow.com/questions/16038414/and-field-not-innull-returns-an-empty-set

I will refactor as follows:

noperm = @rolled_up_versions.select {|version| version.project_id == ts_proj && TsPermission.permission(User.current, version) == TsPermission::NONE }
noperm += @rolled_up_versions.select {|version| version.project_id != ts_proj && !User.current.allowed_to?(:view_issues, version.project)}
if Setting.plugin_redmine_app_timesheets["public_versions"].nil?
    @rolled_up_versions = @rolled_up_versions.where("? IS NULL OR #{Version.table_name}.id NOT IN (?)", noperm, noperm)
end