Defect #26537

Column Project is not longer added by default to the list of default columns for time entries

Added by Marius BALTEANU about 1 year ago. Updated about 12 hours ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Time tracking
Target version:-
Resolution:Fixed Affected version:

Description

After r16814, the column "Project" is no longer added by default to the list of default columns for time entries in the global view.

To reproduce:
  1. Go to "/time_entries"
  2. Observe that the column Project is missing

For issues#index and before this commit, the columns Project was shown by default in the global view.

Attached is a patch that fixes this.

0001-show-project-column-by-default-in-global-time-entry-.patch Magnifier (5.06 KB) Marius BALTEANU, 2018-06-26 08:04


Related issues

Related to Redmine - Feature #26356: Time entry list: set default column options Closed

Associated revisions

Revision 17503
Added by Jean-Philippe Lang about 14 hours ago

Column Project is no longer added by default to the list of default columns for time entries (#26537).

Patch by Marius BALTEANU.

History

#1 Updated by Go MAEDA about 1 year ago

Thank you for the patch but the following error occurs if Setting.time_entry_list_defaults[:column_names] is nil. The value is nil in a fresh installation.

Could you fix the patch?

ActionView::Template::Error (undefined method `map' for nil:NilClass):
     6:   <span>
     7:       <%= label_tag available_tag_id, l(:description_available_columns) %>
     8:       <%= select_tag 'available_columns',
     9:               options_for_select(query_available_inline_columns_options(query)),
    10:               :id => available_tag_id,
    11:               :multiple => true, :size => 10,
    12:               :ondblclick => "moveOptions(this.form.#{available_tag_id}, this.form.#{selected_tag_id});" %>

app/models/time_entry_query.rb:102:in `default_columns_names'
app/models/query.rb:677:in `columns'
app/helpers/queries_helper.rb:109:in `query_available_inline_columns_options'
.
.
.

#2 Updated by Marius BALTEANU about 1 year ago

Thanks for finding this issue, I'll take a look these days. Can you assign this ticket to me, please?

#3 Updated by Go MAEDA about 1 year ago

  • Assignee set to Marius BALTEANU

Marius BALTEANU wrote:

I'll take a look these days. Can you assign this ticket to me, please?

Done.

#4 Updated by Marius BALTEANU about 1 year ago

  • File 26537_show_column_project_by_default_time_entries_index.patch added

I've fixed the issue by using the same logic implementation from issue default columns.

#5 Updated by Marius BALTEANU about 1 year ago

  • File deleted (show_column_project_by_default_time_entries_index.patch)

#6 Updated by Marius BALTEANU about 1 year ago

  • Related to Feature #26356: Time entry list: set default column options added

#7 Updated by Go MAEDA about 1 year ago

  • Target version set to 4.0.0

LGTM. Setting target version to 4.0.0.
Thank you for fixing this issue.

#8 Updated by Marius BALTEANU 3 months ago

  • Assignee deleted (Marius BALTEANU)

#9 Updated by Jean-Philippe Lang 3 months ago

This should be fixed indeed but we should not alter application settings for that.
Defaults for time entries queries are stored in a single setting time_entry_list_defaults and I prefer to keep it like that.

#10 Updated by Marius BALTEANU 3 months ago

  • File deleted (26537_show_column_project_by_default_time_entries_index.patch)

#11 Updated by Marius BALTEANU 3 months ago

  • File 0001-show-project-column-by-default-in-global-time-entry-.patch added
  • Assignee set to Jean-Philippe Lang

Another fix attached without altering application settings. The patch includes also a fix for a missing fixture for #28391.

Jean-Phillipe, if the proposed fix is still not good, I think we can get rid of the methods default_columns_names and default_totalable_names and add the project column to the default list (if is not in a project context) in the build_from_params method. Another ideas I don't have.

#12 Updated by Marius BALTEANU 3 months ago

  • File deleted (0001-show-project-column-by-default-in-global-time-entry-.patch)

#13 Updated by Marius BALTEANU 3 months ago

I had an error in my previous patch and I've removed it for now.

#14 Updated by Marius BALTEANU 3 months ago

  • File 0001-show-project-column-by-default-in-global-time-entry-.patch added

Here is the updated patch.

Jean-Philippe, some mentions regarding my patch:
- I've removed the default columns defined in the default_columns_names and default_totalable_names methods because I found them confusing to have them defined there and also in the application settings. I preferred to keep them only in the default application settings.
- I've added an empty hidden tag in app/views/settings/_timelog.html.erb in order to prevent saving empty columns list from UI
- I've fixed the tests test_index_with_default_query_setting by adding to the settings the key totalable_names which is required.

I think that we can improve the current implementation by validating the presence of the keys totalable_names and column_names for setting time_entry_list_defaults in the "Setting" model. In this way, users will receive a validation error when they will try to save an empty columns list instead of the current message "Successful update.", but without actually saving.

Also, maybe is a good idea after we have the final implementation for this feature to move the settings for issues default list (stored now in 2 settings) to one setting and to have the same implementations.

About my previous note, I'm not sure anymore if the solution with adding the project in the build_from_params method is an option.

#15 Updated by Marius BALTEANU 3 months ago

  • File deleted (0001-show-project-column-by-default-in-global-time-entry-.patch)

#16 Updated by Marius BALTEANU 3 months ago

  • File 0001-show-project-column-by-default-in-global-time-entry-.patch added

#17 Updated by Marius BALTEANU 3 months ago

  • File deleted (0001-show-project-column-by-default-in-global-time-entry-.patch)

#18 Updated by Marius BALTEANU 3 months ago

Sorry for the noise, now is the correct patch attached.

#19 Updated by Jean-Philippe Lang about 14 hours ago

  • Status changed from New to Closed
  • Resolution set to Fixed

Committed, thanks.

#20 Updated by Marius BALTEANU about 12 hours ago

  • Target version deleted (4.0.0)

I'm removing this ticket from the Changelog because no stable versions were been affected by this problem.

Also available in: Atom PDF