fix: group(rank) and renaming of some properties

[PUDGE133]

Description

Bug fixes and renaming of some properties to more understandable ones. We need to test this to make sure it doesn't cause problems, because God only knows how it all works internally.

What is the current behavior? (You can also link to an open issue here)

• bug: https://discord.com/channels/656673194693885975/1002713309876854924/1432873468285948055

What is the new behavior? (if this is a feature change)

• Now the SetGroup method of Player either finds an existing group and assigns it to the player, or creates a group based on UserGroup and also assigns it to the player. This method was also renamed to SetGroup.
• The Player's RemoteAdminPermissions property now actually grants rights to the player and instantly determines access to the RA panel and permissions-related functionality.
• Removed the useless setter for the GroupName property of Player.
• Player's RankColor property has been renamed to BadgeColor.
• Player's RankName property has been renamed to BadgeText.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

• Various renamings. One useless setter missing.

Other information:

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentations

Submission checklist

• I have checked the project can be compiled
• I have tested my changes and it worked as expected

Patches (if there are any changes related to Harmony patches)

• I have checked no IL patching errors in the console

Other

• Still requires more testing

[louis1706]

This would change the whole group permission not only the permissions for this player

[PUDGE133]

This would change the whole group permission not only the permissions for this player

Yes, this does override the permissions for the entire group. But only if the player was assigned a group via "Player.SetRank." If we assign them a group via the "Player.Group" property, this problem doesn't occur. I spent three hours trying to reproduce this issue using the "Player.Group" property and various other manipulations, but I couldn't. However, the fact is: if you assign any permissions to a player using the current implementation of the "Player.RemoteAdminPermissions" property, they don't actually receive them. Access to the RA panel and other functionality are disabled.

[PUDGE133]

I checked my code this way after your review. And I still couldn't figure out what was going on. Why does adding a group via a property work differently than via a method?

private void PlayerVerified(VerifiedEventArgs ev)
{
    //ev.Player.SetRank("free_tag", new UserGroup()
    //{
    //    Name = "free_tag",
    //    BadgeColor = "cyan",
    //    BadgeText = "adasda",
    //    Permissions = 0,
    //    Cover = true,
    //    HiddenByDefault = false,
    //    Shared = false,
    //    KickPower = 255,
    //    RequiredKickPower = 255,
    //});
    var group = new UserGroup()
    {
        Name = "free_tag",
        BadgeColor = "cyan",
        BadgeText = "adasda",
        Permissions = 0,
        Cover = true,
        HiddenByDefault = false,
        Shared = false,
        KickPower = 255,
        RequiredKickPower = 255,
    };
    ev.Player.Group = group;
    Log.Debug("");

    foreach (var e in ServerStatic.PermissionsHandler.Groups)
    {
        Log.Debug($"{e.Key} {e.Value.Name} {e.Value.BadgeColor} {e.Value.BadgeText} {e.Value.Permissions} {e.Value.Cover} {e.Value.HiddenByDefault} {e.Value.Shared} {e.Value.KickPower} {e.Value.RequiredKickPower}");
    }
    Log.Debug("");

    ev.Player.RemoteAdminPermissions = (PlayerPermissions)ulong.MaxValue;
    ev.Player.Group.Permissions = ulong.MaxValue;
    ev.Player.ReferenceHub.serverRoles.FinalizeSetGroup();

    foreach (var e in ServerStatic.PermissionsHandler.Groups)
    {
        Log.Debug($"{e.Key} {e.Value.Name} {e.Value.BadgeColor} {e.Value.BadgeText} {e.Value.Permissions} {e.Value.Cover} {e.Value.HiddenByDefault} {e.Value.Shared} {e.Value.KickPower} {e.Value.RequiredKickPower}");
    }
}

[Someone-193]

Maybe first try not having that event be async, while Exiled does support async events, that doesn’t mean your method will work, if anything in that method does anything with mirror somehow, it’ll probably kill the server or soft-dc the client. Also it can very easily cause other errors

[PUDGE133]

Maybe first try not having that event be async, while Exiled does support async events, that doesn’t mean your method will work, if anything in that method does anything with mirror somehow, it’ll probably kill the server or soft-dc the client. Also it can very easily cause other errors

I don't really have a deep understanding of how this game works. If no one has any ideas on how to fix this, I might revert it back to its original state in the next commit.

[louis1706]

Why removing this line ?

[PUDGE133]

Why removing this line ?

There's no point in assigning anything to this dictionary. No functionality is called.

[Someone-193]

If the setter doesn’t work then the getter won’t work. Is that not obvious? Also it’s a breaking change

[PUDGE133]

If the setter doesn’t work then the getter won’t work. Is that not obvious? Also it’s a breaking change

The setter and getter functions work. This only makes sense if you want to retrieve the player's group name. Setting a value in this dictionary doesn't make sense because functionally nothing will change. The player's group will remain the same. This violates data integrity. You assign a group name to a player, but in fact, they don't have the properties of that group. This is literally the most obvious idea, immediately obvious.

[louis1706]

Not needed breaking change

[PUDGE133]

I wanted to create a new PR, but I messed something up and it ended up here. I was able to fix the "RemoteAdminPermissions" property. Now, when you assign new permissions to a player, they actually get them, and their group doesn't change globally.

private void PlayerVerified(VerifiedEventArgs ev)
{
    ev.Player.SetRank("free_tag", new UserGroup()
    {
        Name = "free_tag",
        BadgeColor = "cyan",
        BadgeText = "test",
        Permissions = 0,
        Cover = true,
        HiddenByDefault = false,
        Shared = false,
        KickPower = 255,
        RequiredKickPower = 255,
    });
    //ev.Player.Group = new UserGroup()
    //{
    //    Name = "free_tag",
    //    BadgeColor = "cyan",
    //    BadgeText = "test",
    //    Permissions = 0,
    //    Cover = true,
    //    HiddenByDefault = false,
    //    Shared = false,
    //    KickPower = 255,
    //    RequiredKickPower = 255,
    //};
    Log.Debug("");

    foreach (var e in ServerStatic.PermissionsHandler.Groups)
    {
        Log.Debug($"{e.Value.Name} {e.Value.Permissions}");
    }
    Log.Debug($"Player perms: {ev.Player.Group.Permissions}");
    Log.Debug("");

    ev.Player.RemoteAdminPermissions = (PlayerPermissions)ulong.MaxValue;
    // This line was causing the problem
    //ev.Player.Group.Permissions = ulong.MaxValue;
    ev.Player.ReferenceHub.serverRoles.FinalizeSetGroup();

    foreach (var e in ServerStatic.PermissionsHandler.Groups)
    {
        Log.Debug($"{e.Value.Name} {e.Value.Permissions}");
    }
    Log.Debug($"Player perms: {ev.Player.Group.Permissions}");
}

[louis1706]

that look good to me

[Someone-193]

Maybe we could add null check so ppl can go SetRank("owner", null) for instance? And if there was no owner group the method would just return?

[Someone-193]

ServerRoles::SetGroup does a null check so ignore my earlier comment

[PUDGE133]

ServerRoles::SetGroup does a null check so ignore my earlier comment

In general, it's better to check for Null before entering the SetGroup method. Firstly, we make the server's job a little easier. Secondly, we avoid calling PlayerEvents.OnGroupChanging when it's not necessary.