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.

Sign up for GitHub

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

Jump to bottom

Allow Users to be disabled/enabled #3639

Merged
merged 8 commits into from
Apr 26, 2022
Merged

Allow Users to be disabled/enabled #3639

merged 8 commits into from
Apr 26, 2022

Conversation

Kukks
Copy link
Member

@Kukks Kukks commented Apr 14, 2022

No description provided.

@Kukks Kukks marked this pull request as ready for review April 15, 2022 08:45
@pavlenex
Copy link
Contributor

Looks good. Two suggestions:

  1. When a disabled user tries to log in, it gets this error:
    Screenshot 2022-04-15 at 13 06 30
    Perhaps we can give a proper error? Your account has been disabled. Please contact server administrator
  2. To me the logic of having disabled tab instead enabled is a bit contradictory. But this isn't a pressing issue. Personal preference would be to have x as red and checkmark as green and have the enabled tab instead of this. Again, this isn't anything pressing, I just think it looks a bit more logical, but don't even mind if it remains like it currently is.
    Screenshot 2022-04-15 at 13 05 52

@Kukks
Copy link
Member Author

Kukks commented Apr 15, 2022

Looks good. Two suggestions:

  1. When a disabled user tries to log in, it gets this error:
    Screenshot 2022-04-15 at 13 06 30
    Perhaps we can give a proper error? Your account has been disabled. Please contact server administrator
  2. To me the logic of having disabled tab instead enabled is a bit contradictory. But this isn't a pressing issue. Personal preference would be to have x as red and checkmark as green and have the enabled tab instead of this. Again, this isn't anything pressing, I just think it looks a bit more logical, but don't even mind if it remains like it currently is.
    Screenshot 2022-04-15 at 13 05 52

fixed

@Kukks Kukks added this to the 1.5.0 milestone Apr 15, 2022
@dstrukt
Copy link
Member

dstrukt commented Apr 21, 2022

@dennisreimann We may want to switch this to the other non-sidebar view in the future. Just noting.

@dstrukt dstrukt mentioned this pull request Apr 21, 2022
Closed
NicolasDorier
NicolasDorier reviewed Apr 25, 2022
View reviewed changes
BTCPayServer.Client/Models/ApplicationUserData.cs
@@ -35,5 +35,7 @@ public class ApplicationUserData
/// </summary>
[JsonConverter(typeof(NBitcoin.JsonConverters.DateTimeToUnixTimeConverter))]
public DateTimeOffset? Created { get; set; }

public bool Disabled { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to Locked

@NicolasDorier
Copy link
Member

@Kukks can I tried to merge the since there was conflict, but I think I broke the build.
Can you renamed Disabled to Locked as well ?

@poor11

This comment was marked as spam.

Sign in to view
@poor11

This comment was marked as spam.

Sign in to view
Kukks and others added 3 commits April 26, 2022 08:11
@Kukks
Merge branch 'master' into user-disabler
4dc2994 
# Conflicts:
#	BTCPayServer/Controllers/GreenField/LocalBTCPayServerClient.cs
@Kukks
fix
9c01bd4
@NicolasDorier
Add enabled user test
2154263
@NicolasDorier NicolasDorier merged commit 273bc78 into master Apr 26, 2022
@NicolasDorier NicolasDorier deleted the user-disabler branch April 26, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dennisreimann dennisreimann dennisreimann left review comments

@NicolasDorier NicolasDorier NicolasDorier left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Kukks @pavlenex @dstrukt @NicolasDorier @poor11 @dennisreimann