Defect #33550
closed
Per role visibility settings for spent time custom fields is not properly checked
Added by Adrien Crivelli almost 4 years ago. Updated about 3 years ago.
0%
Description
The role visibility settings for spent time custom fields introduced by #31859 is completely ignored when editing an issue, eg on /issues/123. So the custom field will incorrectly be shown for any roles.
It will only has an effect when creating/editing a spent time directly, eg on /issues/123/time_entries/new. In this case the custom field will correctly only be shown for selected roles.
I would expect that spent time custom fields are shown/hidden coherently between the two pages.
Custom field configuration¶
Developer vs Reporter¶
The developer, on the left, see the custom field as expected.
The reporter, on the right, should not see the custom field, but he incorrectly sees it when editing an issue (bottom right).
This has been reproduced in production on 4.1.0, and also on a brand new, local Redmine installation with default data loaded.
Environment: Redmine version 4.1.1.stable Ruby version 2.5.1-p57 (2018-03-29) [x86_64-linux-gnu] Rails version 5.2.4.2 Environment production Database adapter Mysql2 Mailer queue ActiveJob::QueueAdapters::AsyncAdapter Mailer delivery smtp SCM: Git 2.17.1 Filesystem Redmine plugins: no plugin installed
Files
Related issues
Updated by Mischa The Evil almost 4 years ago
- Subject changed from Per role visibility settings for spent time custom fields is sometimes ignored to Per role visibility settings for spent time custom fields is ignored on issue edit
- Affected version changed from 4.1.1 to 4.1.0
Looking at the code I think that this issue can be confirmed. Though, I'd first want to rule-out that during your tests any of the users 'Developer' and especially 'Reporter' was accidentally designated as an administrator (please see the admin checkbox in the user settings).
Can you confirm this?
I don't know how good your Ruby on Rails skills are nor if you have a test environment at your disposal, but you can try-out this (preliminary, yet untested) patch against source:/tags/4.1.1 (based on r18358):
app/views/issues/_edit.html.erb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/views/issues/_edit.html.erb b/app/views/issues/_edit.html.erb
index 35527773d..fa8486c54 100644
--- a/app/views/issues/_edit.html.erb
+++ b/app/views/issues/_edit.html.erb
@@ -21,7 +21,7 @@
</div>
</div>
<p><%= time_entry.text_field :comments, :size => 60 %></p>
- <% @time_entry.custom_field_values.each do |value| %>
+ <% @time_entry.editable_custom_field_values.each do |value| %>
<p><%= custom_field_tag_with_label :time_entry, value %></p>
<% end %>
<% end %>
Updated by Adrien Crivelli almost 4 years ago
- File users.png users.png added
- File edit-is-correct-after-patch.png edit-is-correct-after-patch.png added
- File reporter-cannot-submit-time-after-patch.png reporter-cannot-submit-time-after-patch.png added
I can confirm that those two users are not administrator:
And I can also confirm that the patch correctly hides the field for the reporter role when applied on top of r19780 (more recent than what you suggested):
Unfortunately the reporter role cannot submit time spent anymore, because the custom field is correctly hidden, but is incorrectly still required during validation. I expect required, but invisible custom fields to be entirely ignored during validation.
This seems to be in direct relation to my comment from last week #31954-6.
Updated by Mischa The Evil almost 4 years ago
- Assignee set to Mischa The Evil
Adrien, thanks for your quick and useful feedback.
I'll do some more testing later this week.
Updated by Markus Boremski almost 4 years ago
Can confirm this defect on 4.1.1
Also can confirm that #33550#note-1 fixes this defect.
Updated by Adrien Crivelli almost 4 years ago
Markus, I expect experience that even with the patch, a role that do not see the custom field is not able to submit time spent anymore, even though he should be able to. Can you also confirm that unexpected behavior ?
Mischa, did you have an opportunity to have a look at this ?
Updated by Markus Boremski over 3 years ago
Markus, I expect that even with the patch, a role that do not see the custom field is not able to submit time spent anymore, even though he should be able to. Can you also confirm that unexpected behavior ?
We have one field that is required, but it has a default value.
The second field is not requiered and does not have a default value.
This works fine so far.
On our system I can not validate this behavior mentioned by #33550#note-2.
But it sounds logical to me that in case of a reqiered field without a default value there might be an issue with this patch.
Updated by Adrien Crivelli over 3 years ago
Indeed I should have mentioned that my custom field is required but does not have a default value. That might seems a bit unusual, but I feel our use case is reasonable.
Our custom field is meant to select to which customers the time will be billed. Reporters should not be aware of that at all. So we'd like to hide the field for them. On the other hand we want developers to select which customers to bill. It must be billed to someone, so it is required. And we want to force the developers to make a deliberate choice. We cannot say "by default it will be billed to Customer A". So we cannot have a default value.
That makes sense to me. But maybe there's a different way to approach this.
Then you could say that in most cases the reporters probably would not need to log time. So we could live with the patch for a while. But that feels a little bit inconsistent behavior. It's showing a form that can never be valid. Sounds like a bug to me, or at least a suboptimal situation.
Updated by Marius BĂLTEANU over 3 years ago
- Assignee changed from Mischa The Evil to Marius BĂLTEANU
Sorry for not saying anything on this, I'm going to take a look tomorrow on this.
Updated by Marius BĂLTEANU over 3 years ago
- Status changed from New to Confirmed
- Target version set to 4.1.2
Updated by Marius BĂLTEANU over 3 years ago
Adrien Crivelli wrote:
Markus, I expect that even with the patch, a role that do not see the custom field is not able to submit time spent anymore, even though he should be able to. Can you also confirm that unexpected behavior ?
Yes, it's an unexpected behaviour. A custom field not visible for the user should not be required.
I've fixed the first issue (spent time custom field always visible in issue#edit), I'll continue to work on the second issue. In the meantime, I've assigned this issue to 4.1.2 in order to deliver the fixes as soon as possible.
Updated by Marius BĂLTEANU over 3 years ago
- File 0002-Delete-spent-time-custom-field-values-not-visible-by.patch added
- File 0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch added
- Assignee deleted (
Marius BĂLTEANU)
Here are two patches that should fix both issues.
- The first patch fixes the issue as Mischa proposed and add two tests to cover the case.
- The second patch is more delicate because I used another method to remove the custom fields which are not visible for the user. To be more specific, I chose to delete the custom field values (not visible for the user) after the values are assigned. This was the only solution that I found that didn't require more changes in the
safe_attributes=method. What I tried as well is to set to set theTimeEntry.project(because it is required for custom fields visibility), but I encountered some failing tests.
Adrien Besson, Markus Schlichting, can you test the fixes?
Updated by Marius BĂLTEANU over 3 years ago
- Subject changed from Per role visibility settings for spent time custom fields is ignored on issue edit to Per role visibility settings for spent time custom fields is not properly checked
Updated by Adrien Crivelli over 3 years ago
- File missing-red-color-on-custom-fields.png missing-red-color-on-custom-fields.png added
- File working-custom-fields.png working-custom-fields.png added
Thank you for the second patch. I applied both patches on top of 36b5c3204891aa63ff24cf6da434170cdcfdad5e (r20693) successfully, and the behavior is now as expected. Meaning a reporter, on the right, can log time even without submitting a value for the required, no-default and invisible custom field.
I only noticed a small visual inconsistency when submitting an invalid form where custom fields are not highlighted in red like other fields. If you think this is unrelated and should be addressed separately (I think it should be), I'll gladly submit a new issue.
Updated by Marius BĂLTEANU over 3 years ago
Adrien Crivelli wrote:
I only noticed a small visual inconsistency when submitting an invalid form where custom fields are not highlighted in red like other fields. If you think this is unrelated and should be addressed separately (I think it should be), I'll gladly submit a new issue.
Thanks Adrien for testing the fix. Please open a new issue for the inconsistency, I took a look and is not an easy fix.
Updated by Marius BĂLTEANU over 3 years ago
Marius BALTEANU wrote:
Adrien Crivelli wrote:
I only noticed a small visual inconsistency when submitting an invalid form where custom fields are not highlighted in red like other fields. If you think this is unrelated and should be addressed separately (I think it should be), I'll gladly submit a new issue.
Thanks Adrien for testing the fix. Please open a new issue for the inconsistency, I took a look and is not an easy fix.
I've created the issue and I've added a fix for it, please see #34580.
Updated by Marius BĂLTEANU about 3 years ago
- Assignee changed from Jean-Philippe Lang to Go MAEDA
Updated by Go MAEDA about 3 years ago
- Assignee changed from Go MAEDA to Marius BĂLTEANU
test_get_edit_should_not_display_spent_time_custom_field_not_visible is defined twice in test/functional/issues_controller_test.rb.
Could you fix 0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch ?
Updated by Marius BĂLTEANU about 3 years ago
- File 0002-Delete-spent-time-custom-field-values-not-visible-by.patch 0002-Delete-spent-time-custom-field-values-not-visible-by.patch added
- File 0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch 0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch added
- Assignee changed from Marius BĂLTEANU to Go MAEDA
Go MAEDA wrote:
test_get_edit_should_not_display_spent_time_custom_field_not_visibleis defined twice in test/functional/issues_controller_test.rb.Could you fix 0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch ?
Done.
Updated by Marius BĂLTEANU about 3 years ago
- File deleted (
0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch)
Updated by Marius BĂLTEANU about 3 years ago
- File deleted (
0002-Delete-spent-time-custom-field-values-not-visible-by.patch)
Updated by Go MAEDA about 3 years ago
- Status changed from Confirmed to Resolved
- Resolution set to Fixed
Updated by Go MAEDA about 3 years ago
The test fails in 4.1-stable branch. I have not found the cause yet.
$ bin/rails test test/functional/issues_controller_test.rb:4878 Run options: --seed 30553 # Running: F Failure: IssuesControllerTest#test_get_edit_should_display_visible_spent_time_custom_field [/Users/maeda/redmines/4.1-stable/test/functional/issues_controller_test.rb:4890]: Expected exactly 1 element matching "#issue-form select.cf_10", found 0.. Expected: 1 Actual: 0 bin/rails test test/functional/issues_controller_test.rb:4878
Updated by Marius BĂLTEANU about 3 years ago
r19690 unified custom fields classes on 4.2.0 and the class used to assert the element doesn't exist in older versions.
Please apply the following changes:
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index ff7c564ae..e9e0cc255 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -4887,7 +4887,7 @@ class IssuesControllerTest < Redmine::ControllerTest
assert_response :success
- assert_select '#issue-form select.cf_10', 1
+ assert_select '#issue-form select#time_entry_custom_field_values_10', 1
end
def test_get_edit_should_not_display_spent_time_custom_field_not_visible
@@ -4907,7 +4907,7 @@ class IssuesControllerTest < Redmine::ControllerTest
assert_response :success
- assert_select '#issue-form select.cf_10', 0
+ assert_select '#issue-form select.#time_entry_custom_field_values_10', 0
end
Test passes after this change:
root@3d6d6070185e:/work# ruby test/functional/issues_controller_test.rb -n test_get_edit_should_display_visible_spent_time_custom_field Run options: -n test_get_edit_should_display_visible_spent_time_custom_field --seed 7576 # Running: . Finished in 2.137915s, 0.4677 runs/s, 0.9355 assertions/s. 1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
Updated by Go MAEDA about 3 years ago
- Status changed from Resolved to Closed
Committed the patches. Thank you.
Updated by Markus Boremski about 3 years ago
Unfortunately with 4.1.2 we now have an issue regarding this fix.
If we set a default value and the reporting person is not able to edit the field, the default value has no effect.
Should I report a new issue?
Updated by Marius BĂLTEANU about 3 years ago
Markus Boremski wrote:
Unfortunately with 4.1.2 we now have an issue regarding this fix.
If we set a default value and the reporting person is not able to edit the field, the default value has no effect.
Should I report a new issue?
Yes, please. I'll try to catch the fix in the upcoming 4.1.3 which will be release at the end of this month.
Updated by Markus Boremski about 3 years ago
Done -> #35132
Could you please edit title if necessary and maybe link over here.
TY
Updated by Bernhard Rohloff about 3 years ago
- Related to Defect #35132: Custum field default value has no effect if field is inaccessible added