Defect #33550

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

Added by Adrien Crivelli 8 months ago. Updated 14 days ago.

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

0%

Category:Custom fields
Target version:4.1.2
Resolution: Affected version:4.1.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

custom-field-configuration.png (75.1 KB) Adrien Crivelli, 2020-06-04 04:45

developer-vs-reporter.png (155 KB) Adrien Crivelli, 2020-06-04 04:51

users.png (25.5 KB) Adrien Crivelli, 2020-06-08 02:15

edit-is-correct-after-patch.png (45.9 KB) Adrien Crivelli, 2020-06-08 02:22

reporter-cannot-submit-time-after-patch.png (25 KB) Adrien Crivelli, 2020-06-08 02:25

0001-Do-not-display-in-issue-edit-page-spent-time-custom-.patch Magnifier (2.13 KB) Marius BALTEANU, 2020-12-06 12:13

0002-Delete-spent-time-custom-field-values-not-visible-by.patch Magnifier (5 KB) Marius BALTEANU, 2020-12-06 12:13

missing-red-color-on-custom-fields.png (32 KB) Adrien Crivelli, 2020-12-28 02:38

working-custom-fields.png (178 KB) Adrien Crivelli, 2020-12-28 02:42

History

#1 Updated by Mischa The Evil 8 months 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 %>

#2 Updated by Adrien Crivelli 8 months ago

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.

#3 Updated by Mischa The Evil 8 months ago

  • Assignee set to Mischa The Evil

Adrien, thanks for your quick and useful feedback.
I'll do some more testing later this week.

#4 Updated by Markus Boremski 6 months ago

Can confirm this defect on 4.1.1
Also can confirm that #33550#note-1 fixes this defect.

#5 Updated by Adrien Crivelli 6 months 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 ?

#6 Updated by Markus Boremski 4 months 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.

#7 Updated by Adrien Crivelli 4 months 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.

#8 Updated by Marius BALTEANU about 1 month ago

  • Assignee changed from Mischa The Evil to Marius BALTEANU

Sorry for not saying anything on this, I'm going to take a look tomorrow on this.

#9 Updated by Marius BALTEANU about 1 month ago

  • Status changed from New to Confirmed
  • Target version set to 4.1.2

#10 Updated by Marius BALTEANU about 1 month 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.

#11 Updated by Marius BALTEANU about 1 month ago

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?

#12 Updated by Marius BALTEANU about 1 month 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

#13 Updated by Marius BALTEANU about 1 month ago

  • Assignee set to Jean-Philippe Lang

#14 Updated by Adrien Crivelli 27 days ago

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.

#15 Updated by Marius BALTEANU 14 days 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.

#16 Updated by Marius BALTEANU 14 days 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.

Also available in: Atom PDF