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

Rewriting permission system to use UUIDs and Ranks #1290

Merged
merged 41 commits into from Aug 26, 2014
Merged

Rewriting permission system to use UUIDs and Ranks #1290

merged 41 commits into from Aug 26, 2014

Conversation

madmaxoft
Copy link
Member

As outlined in the forum http://forum.mc-server.org/showthread.php?tid=1540 , this PR changes the permission system to use UUIDs instead of playernames, and at the same time is refactored to use SQLite for the storage, and an entire new set of API for management.

  • SQLite-based storage
  • Lua API for manipulating the ranks
  • Rewrite the Core WebAdmin to support the new system
  • Rewrite the Core commands to support the new system
  • Migration from old INI files to new DB
  • Make the Players actually use this system
  • Document the Lua API
  • Remove the old API / classes
  • Make cMojangAPI update the cached PlayerNames in the ranks
  • Have one rank as the default for players without a rank
  • Create default ranks when there are none in the DB and none to migrate

@madmaxoft madmaxoft self-assigned this Aug 5, 2014
m_DB("Ranks.sqlite", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE)
{
// Create the DB tables, if they don't exist:
m_DB.exec("CREATE TABLE IF NOT EXISTS Rank (RankID, Name, MsgPrefix, MsgPostfix, MsgNameColorCode)");
Copy link
Member

Choose a reason for hiding this comment

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

Does SQLite specify a default type? I thought you had to specify one when creating tables.

Copy link
Member Author

Choose a reason for hiding this comment

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

SQLite doesn't care about types much, you can store just about any datatype in any column. It has some little preferences ("affinity"). By default the fields tend to be of "string" type, which suits us for most.

@worktycho worktycho added this to the UUID Switch milestone Aug 5, 2014
@archshift
Copy link
Contributor

Shouldn't MsgPostfix be MsgSuffix?

@madmaxoft
Copy link
Member Author

Yup, you're right

@madmaxoft
Copy link
Member Author

I'm not sure if I'm doing this right. The SQL seems clunky.

@worktycho
Copy link
Member

The SQL looks normal for a fully normalised database.
However would it be possible to extract the test code into an actual test?

@madmaxoft
Copy link
Member Author

The test code, as it is now, doesn't do as much checking, it's more like only for seeing if the code even works. Do you think it'd be worth to keep it as a test? It'd be easier to make the test into a plugin, once all the functions are Lua-exported.

@worktycho
Copy link
Member

Its more that once we have one test it becomes massively easier to write more for that class as the hard part of getting it to compile separately is done.

@worktycho
Copy link
Member

Also firing up the server to run a test is expensive so I think it is better to have the tests outside plugins where possible.

@madmaxoft
Copy link
Member Author

Gentlemen, I'd like your comments on the cRankManager API - do you think it's okay? Is there anything missing? See the Core PR cuberite/Core#83 for an example usage of the API.

@NiLSPACE
Copy link
Member

NiLSPACE commented Aug 9, 2014

I took a quick look, and I think it's great.


// Get the permissions:
AString MsgPrefix, MsgSuffix, MsgNameColorCode;
cRoot::Get()->GetRankManager().GetPlayerMsgVisuals(PlayerUUID, MsgPrefix, MsgSuffix, MsgNameColorCode);
Copy link
Member

Choose a reason for hiding this comment

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

Unchecked return value

@madmaxoft
Copy link
Member Author

I think this is now feature-complete. Anyone up for reviewing this whole lot?





Copy link
Contributor

Choose a reason for hiding this comment

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

6 instead of 5 empty lines?

@madmaxoft
Copy link
Member Author

You're being really thorough :)

@madmaxoft
Copy link
Member Author

Oh, the default rank is not being applied, yet. Gotta fix that.

madmaxoft added a commit that referenced this pull request Aug 26, 2014
Rewriting permission system to use UUIDs and Ranks
@madmaxoft madmaxoft merged commit be32c05 into master Aug 26, 2014
@NiLSPACE
Copy link
Member

Does git already download the latest version of the Core or do we need to update the submodule?

@madmaxoft
Copy link
Member Author

I have no idea. I've updated the Core reference in the branch, so theoretically it should give you the updated version, but then the Core branch has been merged afterwards, so it won't be the merged branch, it'll be the Ranks branch just before merging. Best to update manually :)

@madmaxoft madmaxoft deleted the Ranks branch August 26, 2014 13:57
@NiLSPACE
Copy link
Member

I just downloaded it and it gave the latest version :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants