search

alphanodes / additionals

Redmine plugin for easy customization of settings, text and content display by using personal or role-based dashboards (drag&drop), providing wiki macros and act as library for other plugins.
https://www.redmine.org/plugins/additionals
GNU General Public License v2.0
131 stars 43 forks source link

Update projects_controller_patch.rb #133

Closed picman closed 2 years ago

picman commented 2 years ago

If @dashboard is not found, the code @dashboard.content_project = @project triggers an error.

alexandermeindl commented 2 years ago

Hi @picman it should never be the case, that no dashboard is available. The default dashboard is not removable. If there is no dashboard found, the error occurred somewhere else. In migration the default dashboards are created for welcome and project page, see https://github.com/AlphaNodes/additionals/blob/main/db/migrate/005_create_dashboard_defaults.rb

With this patch, the project overview page does not show any content, if no dashboard is found. In my opinion it is better to trigger an error instead of hide a problem.

If you can provide a test, which makes your error reproducible, we can try to find the real problem.

picman commented 2 years ago

The problem occurs in a functional test of show action of Projects controller in an another plugin if the dashboard fixtures are not loaded and therefore no default dashboard is available. In other words I can't test other plugins if Additionals plugin is present and its test weren't run to initialize a default dashboard. So you are right that it cannot happen in normal run but there are cases where it might cause problems. If you look on the original code, there already is a condition dealing with null dashoboard. What is the purpose of the ampersand on the line 44 if you access the same variable on the line 42 without any check?

 42 @dashboard.content_project = @project
 43 recently_used_dashboard_save @dashboard, @project
 44 @can_edit = @dashboard&.editable?
alexandermeindl commented 2 years ago

The problem occurs in a functional test of show action of Projects controller in an another plugin if the dashboard fixtures are not loaded and therefore no default dashboard is available. In other words I can't test other plugins if Additionals plugin is present and its test weren't run to initialize a default dashboard. So you are right that it cannot happen in normal run but there are cases where it might cause problems. If you look on the original code, there already is a condition dealing with null dashoboard. What is the purpose of the ampersand on the line 44 if you access the same variable on the line 42 without any check?

 42 @dashboard.content_project = @project
 43 recently_used_dashboard_save @dashboard, @project
 44 @can_edit = @dashboard&.editable?

You are right, this ampersand is not required - I'll remove it. We solved the problem with other plugin tests by adding dashboard fixtures to theses plugins. With this workaround we can also test some dashboard blocks, which other plugins are providing.

picman commented 2 years ago

I believe that a better workaround would be if the other plugins wouldn't have to add dashboard fixtures. But I can live with that. Don't worry ;-)

alexandermeindl commented 2 years ago

I believe that a better workaround would be if the other plugins wouldn't have to add dashboard fixtures. But I can live with that. Don't worry ;-)

You are right, but at the moment I do not have another solution for that. All logic is based on the fact, that a dashboard is always available. If no dashboard would be loaded, other tests would be fail (most likely) - because there is no content on project overview page.