Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Username realname proposal #1351

Merged
merged 6 commits into from May 30, 2018

Conversation

@atrol
Copy link
Member

commented May 19, 2018

Proposal to fix https://mantisbt.org/bugs/view.php?id=24139

I don't think that my proposal https://mantisbt.org/bugs/view.php?id=24139#c59327 is something that can be done in short term, because of the open questions from https://mantisbt.org/bugs/view.php?id=24139#c59831

The general question is if we want to have again that $g_show_user_realname_threshold is not considered if $g_show_realname = ON;

If so, this PR is a try to

@atrol atrol changed the title [WIP not tested]Username realname proposal [WIP not tested] Username realname proposal May 19, 2018

@atrol atrol self-assigned this May 19, 2018

@atrol atrol changed the title [WIP not tested] Username realname proposal [WIP] Username realname proposal May 21, 2018

@atrol atrol changed the title [WIP] Username realname proposal Username realname proposal May 22, 2018

@atrol atrol force-pushed the atrol:username-realname-proposal branch 2 times, most recently from 2635c64 to 109d8c4 May 23, 2018

@vboctor

This comment has been minimized.

Copy link
Member

commented May 23, 2018

@atrol it would be good if you can write a comment that explains the behavior. Think of it as a short spec. I worry that we are going for a configuration that may be confusing (realname == OFF, yet threshold causes realnames to be visible) and not consistent across pages/features. Irrespective of short term bugs, if a user can see realname by default, they should see it everywhere in view issue page, email, etc. If a user can't see it, it should be everywhere. If we want fine controls, between email and view page, then should these be their own config options, etc?

@atrol

This comment has been minimized.

Copy link
Member Author

commented May 23, 2018

may be confusing (realname == OFF, yet threshold causes realnames to be visible)

I don't think it's that much confusing, as the threshold is just used at a single place if realname == OFF.
After merging my PR, the behavior is again what is documented and what worked for many years.

 * This specifies the access level that is needed to see realnames on user view
 * page

if a user can see realname by default, they should see it everywhere in view issue page, email, etc

Have a look at the table below.
I think it is a good compromise between backward compatibility and adding some help to find out the username for @ mentions.

If we want fine controls, between email and view page, then should these be their own config options, etc?

Right, but IMO we need a short term solution.
I am not able to invest even more time in this topic at the moment.

Expected behavior after merging the PR

Display location show_realname ON show_realname OFF
"Reporter" and "Assigned To" on view.php Realname + Username in tooltip Username
"Reporter" and "Assigned To" in E-Mail Notifications Realname Username
"Reporter" and "Assigned To" in CSV Export Realname Username
"Reporter" and "Assigned To" in Excel Export Realname Username
"Users monitoring this issue" on view.php Realname + Username in tooltip Username
History on view.php Realname + Username in tooltip Username
History in E-Mail Notifications Realname Username
Drop down lists to select a single user, e.g. "Assign To" or "Simple Filters" Realname + (Username) Username
Lists to select multiple users, e.g. "Send Reminder" or "Advanced Filters" Realname + (Username) Username
Timeline Realname + Username in tooltip Username
"Reporter" and "Assigned To" columns on view_all_bug_page.php Realname + Username in tooltip Username
"Status" column on view_all_bug_page.php if show_assigned_names = ON Realname + Username in tooltip Username
"Real Name" on view_user_page Realname Realname or nothing, depending on show_user_realname_threshold
@dregad
dregad approved these changes May 24, 2018
Copy link
Member

left a comment

I didn't actually test this (no time, sorry), but I fully support the approach, and after a quick look at the code, I have no particular remarks.

Many thanks @atrol for working on this.

@vboctor

This comment has been minimized.

Copy link
Member

commented May 25, 2018

Thanks @atrol for the detailed description. The behavior described in the table seems reasonable. However:

  • When do we consider show realname to be ON vs OFF - The implemented behavior here shows realname even if it is OFF which seems to be inconsistent with other configs. Why wouldn't we require show_realname = ON and threshold greater or equal to show_realname_threshold?

  • In case of show realname enabled (based on whatever criteria we agree on in the previous bullet) - Should we show realname (username) in case of email notifications, csv export, and Excel export? This would be consistent with html cases where we should Realname + username in tooltip or dropdown where we show realname (username).

@atrol

This comment has been minimized.

Copy link
Member Author

commented May 25, 2018

The implemented behavior here shows realname even if it is OFF which seems to be inconsistent with other configs

You mean this row with it

Display location show_realname ON show_realname OFF
"Real Name" on view_user_page Realname Realname or nothing, depending on show_user_realname_threshold

Visit this page on our bug tracker.
Do you really want to see any other thing than the real name of atrol in field Real Name on this page if you are logged in as user vboctor?

Why wouldn't we require show_realname = ON and threshold greater or equal to show_realname_threshold?

As told before

  • the behavior is what is documented specifies the access level that is needed to see realnames on user view page
  • the behavior is what we had for many years, which means that users don't have to set show_realname_threshold (default is NOBODY) to see real names if show_realname = ON

Should we show realname (username) in case of email notifications, csv export, and Excel export?

Not at the moment, as there are users relying on the former behavior due to security reasons where usernames were not visible at various places if show_realname = ON.
Please read all the discussions in the affected and related issues.

Again: This is a compromise, as long as there is no one who implements https://mantisbt.org/bugs/view.php?id=24139#c59327 + adds a new option to protect user names (something like $g_show_user_username_threshold )+ deals with empty real names + deals with non unique real names.

All I could offer to replace this PR, is a new PR that reverts even more to the old behavior but keeps just your refactoring to have less places where we deal with user/real names.

@vboctor

This comment has been minimized.

Copy link
Member

commented May 30, 2018

The implemented behavior here shows realname even if it is OFF which seems to be inconsistent with other configs

You mean this row with it

What I meant that for other options we honor the ON/OFF feature first and only if ON we check associated threshold. Or we don't have ON/OFF and just rely on threshold. Of course, I expect that the Real Name field on the user view page to only show Real Name (if visible to logged in user). I understand the inconsistent here is motivated by backward compatibility.

the behavior is what we had for many years, which means that users don't have to set show_realname_threshold (default is NOBODY) to see real names if show_realname = ON

I think if the desired behavior is possible, it is OK to ask changing the value of the config options. We have already changed the behavior for several releases, so it is not like it is a new change. We can also make this clear in the release notes.

This is a compromise

I understand that, and I'm generally OK with your proposal here. I don't think we should revert. The old code is a mess. But I'm of the opinion that an inconsistent behavior should be fixed over time and that not every MantisBT user will be on board with the change. So even with merging this PR shouldn't mean that we shouldn't have future changes that will drive consistency. For cases where we want to provide flexibility, we can have config options, but we should eventually get to a point where we are consistent and forward looking by default.

In summary, I'm OK with this PR, but that doesn't exclude future changes to drive consistency even if there will be requests not to change the behavior. I would also like with future changes to get to a point that calling code not have to replicate business logic of whether to show realname or not, but depend on call to user API.

Anyway, thanks @atrol for putting this together.

atrol added 6 commits May 19, 2018
Unifiy show_realname handling
Issue #24139
Issue #24435
Enhance handling of realname visibility
Partially return to pre 2.12.0 behavior
- make show_realname independant from show_user_username_threshold
- use show_user_username_threshold just on view_user_page and SOAP API

- enhance realname access by allowing it also if show_realname is set to ON
- refactor code
Send usernames in e-mail notifications again based on show_realnames
This reverts the changes from #24239 that have been needed as we
were no longer able to protect realnames by show_user_realname_threshold.

The change assumes that we agree, that show_realnames = ON allows
any user to see the realnames.

show_user_realname_threshold is just used on view user page
if show_realnames = OFF (behavior before version 2.12.0)

Issue #24432
Send usernames in history of e-mail notifications based on show_realn…
…ames

This reverts the changes from #24167 that have been needed as we
were no longer able to protect realnames by show_user_realname_threshold.

The change assumes that we agree, that show_realnames = ON allows
any user to see the realnames.

show_user_realname_threshold is just used on view user page
if show_realnames = OFF (behavior before version 2.12.0)

Issue #24432

@atrol atrol force-pushed the atrol:username-realname-proposal branch from 7aef1f8 to 97fff18 May 30, 2018

@atrol

This comment has been minimized.

Copy link
Member Author

commented May 30, 2018

I don't think we should revert. The old code is a mess.

Right, that's why I kept as much as possible of your refactoring in the PR.

But I'm of the opinion that an inconsistent behavior should be fixed over time and that not every MantisBT user will be on board with the change.

Right, we cannot know if a user relies on an undocumented feature (e.g. don't display usernames in e-mail notifications).
For most of the users (including myself) #1349 would have been enough.

I'm OK with this PR, but that doesn't exclude future changes to drive consistency even if there will be requests not to change the behavior.

+1

@atrol atrol merged commit 97fff18 into mantisbt:master May 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@atrol atrol deleted the atrol:username-realname-proposal branch May 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.