プロジェクト

全般

プロフィール

Vote #80905

完了

Per role visibility settings for spent time custom fields is not properly checked

Admin Redmine さんが3年以上前に追加. 3年以上前に更新.

ステータス:
Closed
優先度:
通常
担当者:
-
カテゴリ:
Custom fields_14
対象バージョン:
開始日:
2022/05/09
期日:
進捗率:

0%

予定工数:
category_id:
14
version_id:
162
issue_org_id:
33550
author_id:
1475
assigned_to_id:
332
comments:
30
status_id:
5
tracker_id:
1
plus1:
0
affected_version:
closed_on:
affected_version_id:
127
ステータス-->[Closed]

説明

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.

h1. Custom field configuration

!custom-field-configuration.png!

h1. 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).

!developer-vs-reporter.png!

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


journals

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):
<pre><code class="diff">
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 %>

</code></pre>
--------------------------------------------------------------------------------
I can confirm that those two users are *not* administrator:

!users.png!

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):

!edit-is-correct-after-patch.png!

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.

!reporter-cannot-submit-time-after-patch.png!
--------------------------------------------------------------------------------
Adrien, thanks for your quick and useful feedback.
I'll do some more testing later this week.
--------------------------------------------------------------------------------
Can confirm this defect on 4.1.1
Also can confirm that #33550#note-1 fixes this defect.
--------------------------------------------------------------------------------
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 ?
--------------------------------------------------------------------------------
> 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.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
Sorry for not saying anything on this, I'm going to take a look tomorrow on this.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
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 version:"4.1.2" in order to deliver the fixes as soon as possible.
--------------------------------------------------------------------------------
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 the @TimeEntry.project@ (because it is required for custom fields visibility), but I encountered some failing tests.

@Adrien, @Markus, can you test the fixes?
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
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.

!working-custom-fields.png!

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.

!missing-red-color-on-custom-fields.png!
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
@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 ?
--------------------------------------------------------------------------------
Go MAEDA wrote:
> @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 ?

Done.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
The test fails in 4.1-stable branch. I have not found the cause yet.

<pre>
$ 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
</pre>
--------------------------------------------------------------------------------
I will take a look.
--------------------------------------------------------------------------------
r19690 unified custom fields classes on version:"4.2.0" and the class used to assert the element doesn't exist in older versions.

Please apply the following changes:
<pre><code class="diff">
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
</code></pre>

Test passes after this change:
<pre>
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
</pre>
--------------------------------------------------------------------------------
Committed the patches. Thank you.
--------------------------------------------------------------------------------
Unfortunately with version:"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?
--------------------------------------------------------------------------------
Markus Boremski wrote:
> Unfortunately with version:"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 version:"4.1.3" which will be release at the end of this month.
--------------------------------------------------------------------------------
Done -> #35132
Could you please edit title if necessary and maybe link over here.
TY
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------


related_issues

relates,New,35132,Custum field default value has no effect if field is inaccessible

Admin Redmine さんが3年以上前に更新

  • カテゴリCustom fields_14 にセット
  • 対象バージョン4.1.2_162 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0