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

Fixed unban not overwriting banlist.txt #228

Merged
merged 8 commits into from
Aug 13, 2022

Conversation

ScureX
Copy link
Contributor

@ScureX ScureX commented Jul 24, 2022

This closes #165.
In the current state it only works with #227!

When unbanning it comments out the uid and adds a comment with the local time when the person was unbanned. Gives a nice overview i think, but it only works with #227 since that adds the comment functionality.
image


@ScureX
Copy link
Contributor Author

ScureX commented Jul 24, 2022

Leaving this here for everyone that thinks this may be a computation problem:
image

considering our player counts, 200 lines taking <1ms and how rare it is to unban a user i think this is not a problem. if we suddenly get more players than vanilla we can think about improving this

@GeckoEidechse
Copy link
Member

I gotta say I really like the idea of reusing your previous PR to just comment out bans. ^^

Also, making it dependent on the previous PR is a smart way to get that PR merged faster :trollface:

@GeckoEidechse
Copy link
Member

Just to be sure, you tested, banning, unbanning, re-banning, and re-unbanning the same UID?

@ScureX
Copy link
Contributor Author

ScureX commented Jul 24, 2022

I gotta say I really like the idea of reusing your previous PR to just comment out bans. ^^

spoke to wolf about it, i feel like this is a really nice thing so the uid isnt just "gone" but you have an overview of what you did and when

Just to be sure, you tested, banning, unbanning, re-banning, and re-unbanning the same UID?

those 2 prs arent on the same branch so i only tested their independant quirks 💀 ill make a new branch where i merge the 2 and test it fully!

what im like 99.9% sure of if going to happen:

  • banning:

just adds the uid at the bottom of a file, probably with one line too many if you havent fucked with the banlist thru the editor and only used commands, but rather 1 line too much than 2 uids sticking together (had that happen before)

  • unbanning:

since i go thru the file in a loop and check every line for the uid that should be unbanned, even duplicate, active uids should be commented out, just in different places

  • re-banning:

re-banning will NOT comment out the unbanned uid! it wil just add the uid at the bottom again, which i honestly prefer. that way you have like a timeline of what happened when and you can recheck and see how often someone has been unbanned (like a "you have x chances till perm ban" thing maybe)

  • re-unbanning

re-unbanning will (because of the loop) once again just comment out every line that has the right (active) uid

@GeckoEidechse i will test this all again tho to be 100% sure and ping u here again :)

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Jul 24, 2022

  • re-banning:

re-banning will NOT comment out the unbanned uid! it wil just add the uid at the bottom again, which i honestly prefer. that way you have like a timeline of what happened when and you can recheck and see how often someone has been unbanned (like a "you have x chances till perm ban" thing maybe)

This is what I would expect and also prefer myself personally.

  • re-unbanning

re-unbanning will (because of the loop) once again just comment out every line that has the right (active) uid

This is what I'm looking for. I haven't gotten the chance to read fully through the code so I just want to make sure that I comments out the right UID and doesn't just somehow grab the earlier one and just add another # to the already commented out version of the UID ^^

@ScureX
Copy link
Contributor Author

ScureX commented Jul 24, 2022

This is what I'm looking for. I haven't gotten the chance to read fully through the code so I just want to make sure that I comments out the right UID and doesn't just somehow grab the earlier one and just add another # to the already commented out version of the UID ^^

std::string modLine = line; // copy the line into a free var that we can fuck with, line will be the original

// remove tabs which shouldnt be there but maybe someone did the funny
modLine.erase(std::remove(modLine.begin(), modLine.end(), '\t'), modLine.end());
// remove spaces to allow for spaces before uids
modLine.erase(std::remove(modLine.begin(), modLine.end(), ' '), modLine.end());

// ignore line if first char is # or empty line, just add it
if (line.front() == BANLIST_COMMENT_CHAR || modLine == "")
{
  banlistText.push_back(line);
  continue;
}

to spare you from reading through the whole file: this is the relevant part. it will do that for every line unless the first non-space/non-tab character is # to make sure its an active uid. then it just compares uids and comments out or not. that way it will comment out all the active uids while ignoring comments :P

uids with inline comments werent able to unban due to whitespaces
@ScureX
Copy link
Contributor Author

ScureX commented Jul 24, 2022

alright so i fixed a small issue where unbanning a uid with inline comment wouldnt work because of whitespaces.

ive tested banning, unbanning, re.banning and re.unbanning without issues but id be glad if others could confirm!

@GeckoEidechse
example of a full file with the 2 uids at the bottom following banning, unbanning, re-banning and adding a comment, and re-unbanning:
image

@ScureX
Copy link
Contributor Author

ScureX commented Jul 25, 2022

for testing purposes i merged the 2 pr branches into this branch https://github.com/ScureX/NorthstarLauncher/tree/commentsAndUnbanPRs so if anyone is doing a review or testing shit feel free to use it :)

@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code depends on another PR Blocked until the PR it depends on is merged labels Jul 25, 2022
@ScureX ScureX changed the title Fixed unban not overwriting banlist.txt Fixed unban not overwriting banlist.txt + added banlist hotreload Aug 10, 2022
@ScureX
Copy link
Contributor Author

ScureX commented Aug 10, 2022

alright so i made the banlist editable by closing it everywhere, it gets reloaded when a player joins. not really a need to reload it when a player gets banned since on join it get checked anyways

@ScureX
Copy link
Contributor Author

ScureX commented Aug 10, 2022

moved hot reload to #233

@ScureX ScureX changed the title Fixed unban not overwriting banlist.txt + added banlist hotreload Fixed unban not overwriting banlist.txt Aug 10, 2022
@GeckoEidechse GeckoEidechse added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed depends on another PR Blocked until the PR it depends on is merged labels Aug 10, 2022
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Worked in testing by banning, unbanning, and re-banning a player a bunch of times with them (trying to) join in-between.

@GeckoEidechse GeckoEidechse removed the needs testing Changes from the PR still need to be tested label Aug 10, 2022
@GeckoEidechse
Copy link
Member

Bob said it's chill :D

@GeckoEidechse GeckoEidechse merged commit df056e7 into R2Northstar:main Aug 13, 2022
@GeckoEidechse GeckoEidechse removed needs code review Changes from PR still need to be reviewed in code almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Aug 13, 2022
GeckoEidechse pushed a commit that referenced this pull request Aug 14, 2022
* fixed unban

* changed timestamp for gecko

* fixed unbanning with inline comment

uids with inline comments werent able to unban due to whitespaces
This was referenced Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

unban command not working
2 participants