Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Change description of role in RequireRole method - #1831

Merged
Eilon merged 1 commit into
aspnet:masterfrom
woeterman94:patch-1
Nov 15, 2018
Merged

Eilon merged 1 commit into
aspnet:masterfrom
woeterman94:patch-1

Conversation

@woeterman94

Copy link
Copy Markdown
Contributor

The description of the pram roles feels ambiguous. Make it clear that the user should belong to one of the roles and not all of them.
As documented: https://docs.microsoft.com/en-us/aspnet/core/security/authorization/roles?view=aspnetcore-2.1

The description of the pram roles feels ambiguous. Make it clear that the user should belong to one of the roles and not all of them.
As documented: https://docs.microsoft.com/en-us/aspnet/core/security/authorization/roles?view=aspnetcore-2.1
@Eilon
Eilon requested a review from blowdart July 31, 2018 16:33
@Eilon

Eilon commented Jul 31, 2018

Copy link
Copy Markdown
Contributor

@blowdart can you share your thoughts on this new phrasing?

I agree the old phrasing was misleading. I'm not sure I'm crazy about the new wording either, but I do think it's better.

@blowdart

Copy link
Copy Markdown
Member

It's better. How about "The collection/array of roles where membership of at least one of the roles fulfills the requirement"? Or is that too wordy?

@woeterman94

Copy link
Copy Markdown
Contributor Author

That's too wordy, and perhaps more confusing.

@Eilon
Eilon merged commit 1c4a695 into aspnet:master Nov 15, 2018
@Eilon

Eilon commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

Discussed in triage - this change is better than what was previously there. Thanks for the PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants