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

Core/Auth: Cleanup and merge mute system/history #15588

Closed
wants to merge 1 commit into from
Closed

Core/Auth: Cleanup and merge mute system/history #15588

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 23, 2015

Cleanup and merge mute system/history, everything is handled in account_muted.

@ghost
Copy link
Author

ghost commented Sep 24, 2015 

$ mysql -utrinity -ptrinity auth < sql/base/auth_database.sql

ERROR 1072 (42000) at line 119: Key column 'guid' doesn't exist in table
The command "mysql -utrinity -ptrinity auth < sql/base/auth_database.sql" exited with 1.

The same error as on your previous PR. Maybe not a good idea to rename the primary key guid ?

@ghost
Copy link
Author

ghost commented Sep 24, 2015

I just forgot to rename it on the primary key definition.

@jackpoz
Copy link
Member

jackpoz commented Sep 24, 2015

No description provided.

Please provide a detailed description of the Pull Request

@ghost
Copy link
Author

ghost commented Sep 24, 2015

Done.

Naios
Naios reviewed Sep 27, 2015
View reviewed changes
sql/base/auth_database.sql
@@ -549,4 +547,4 @@ UNLOCK TABLES;
/*!40101 SET COLLATION_CONNECTION=@OLD_COLLATION_CONNECTION */;
/*!40111 SET SQL_NOTES=@OLD_SQL_NOTES */;

Copy link
Contributor

Choose a reason for hiding this comment

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

The entry in the updates table is missing for your update 2015_09_23_15588_auth.sql.

Copy link
Author

Choose a reason for hiding this comment

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

I did not understand what you mean.

Copy link

Choose a reason for hiding this comment

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

on your MySQL server, run the following command:

SELECT * FROM `auth`.`updates`;

Result from the standard database contents as of today:

name                    hash                                      state               timestamp   speed  
----------------------  ----------------------------------------  --------  -------------------  --------
2014_11_10_00_auth.sql  0E3CB119442D09DD88E967015319BBC8DAFBBFE0  ARCHIVED  2015-03-21 22:44:12         0
2014_11_10_01_auth.sql  327E77A1DA3546D5275AB249915DD57EDD6FDD3D  ARCHIVED  2015-03-21 22:44:12         0
2014_12_10_00_auth.sql  821703A96D80F9080074852B5A46E2909C9562EA  ARCHIVED  2015-03-21 22:44:12         0
2014_12_21_00_auth.sql  CE2E5D2CD82E79C25294539ADED27A1429105B43  ARCHIVED  2015-03-21 22:44:12         0
2015_03_20_00_auth.sql  E8C5B74BB45F0F35DEC182C72BACF435C7066FB0  ARCHIVED  2015-03-21 22:44:12         0
2015_03_20_01_auth.sql  862961815354DA2746F5F71FBC8155F57CBE75AB  ARCHIVED  2015-03-21 22:44:12         0
2015_03_20_02_auth.sql  33E4F94086590768EF5D4855DD43D7DE7C06ADA4  ARCHIVED  2015-03-21 22:44:51         0
2015_08_21_00_auth.sql  C31A9E1D28E11B60BE8F8198637DD51F6D75123F  ARCHIVED  2015-10-06 01:16:19         0
2015_11_07_00_auth.sql  0ACDD35EC9745231BCFA701B78056DEF94D0CC53  RELEASED  2015-12-12 08:14:15       169

Naios is trying to say that your SQL auth update file is missing on the last line in this table.

Edit: updated with current status from auth.updates as of 2015-12-12.

@ghost
Copy link
Author

ghost commented Sep 28, 2015

I should insert using a query or changing the auth_database.sql?
One question, because there are several other updates that are not in this table auth.updates?

@ghost
Copy link
Author

ghost commented Oct 5, 2015

added lines to updates table.

@ghost ghost changed the title Core/Auth: Cleanup mute system Core/Auth: Cleanup and merge mute system/history Oct 9, 2015
@ghost
Copy link
Author

ghost commented Oct 12, 2015

@tkrokli
Can you help me ?

Travis fails with:

Fatal error:
The Database Updater is broken for the 'auth' database,
because the applied update is missing in the 'auth'.'updates' table.

But It already in updates table.

Edit: I found the problem, just another typo in sql file.

@ghost
Copy link
Author

ghost commented Oct 12, 2015

Sorry I could not reply to your question sooner, I think that issue happened a few days ago where the TC source database was not updated and therefore made Travis say that all PRs could not compile. I think it was fixed yesterday, I don't remember exactly when. Glad to see that your PR is OK now.

@MitchesD
Copy link
Contributor

All auth/character update files must be added to updates table, it was changed some weeks ago and now it required (to prevent auto-updater problems with re-applying these updates)

@Aokromes
Copy link
Member

Aokromes commented Nov 8, 2015

This branch has conflicts that must be resolved
Use the command line to resolve conflicts before continuing.

@ghost
Copy link
Author

ghost commented Nov 8, 2015

I will fix as soon as possible, thank you.

@ghost
Copy link
Author

ghost commented Nov 8, 2015

@Aokromes

Done.

@jackpoz
Copy link
Member

jackpoz commented Dec 12, 2015

the current update sql script is deleting all the mute data stored in the database. please modify the script to migrate the data to the new structure

@ghost
Copy link
Author

ghost commented Dec 12, 2015

Branch mthsena:3.3.5 is based on commit 7d5e710, making it 137 commits behind TrinityCore:3.3.5.

@jackpoz
Copy link
Member

jackpoz commented Dec 12, 2015

It doesn't matter on which commit it's based on or how many commits it's behind

@ghost
Copy link
Author

ghost commented Dec 17, 2015

@jackpoz
Done.

jackpoz
jackpoz reviewed Dec 17, 2015
View reviewed changes
src/server/database/Database/Implementation/LoginDatabase.cpp
@@ -87,7 +87,7 @@ void LoginDatabaseConnection::DoPrepareStatements()
PrepareStatement(LOGIN_GET_USERNAME_BY_ID, "SELECT username FROM account WHERE id = ?", CONNECTION_SYNCH);
PrepareStatement(LOGIN_SEL_CHECK_PASSWORD, "SELECT 1 FROM account WHERE id = ? AND sha_pass_hash = ?", CONNECTION_SYNCH);
PrepareStatement(LOGIN_SEL_CHECK_PASSWORD_BY_NAME, "SELECT 1 FROM account WHERE username = ? AND sha_pass_hash = ?", CONNECTION_SYNCH);
PrepareStatement(LOGIN_SEL_PINFO, "SELECT a.username, aa.gmlevel, a.email, a.reg_mail, a.last_ip, DATE_FORMAT(a.last_login, '%Y-%m-%d %T'), a.mutetime, a.mutereason, a.muteby, a.failed_logins, a.locked, a.OS FROM account a LEFT JOIN account_access aa ON (a.id = aa.id AND (aa.RealmID = ? OR aa.RealmID = -1)) WHERE a.id = ?", CONNECTION_SYNCH);
PrepareStatement(LOGIN_SEL_PINFO, "SELECT a1.username, a2.gmlevel, a1.email, a1.reg_mail, a1.last_ip, DATE_FORMAT(a1.last_login, '%Y-%m-%d %T'), a3.mutetime, a3.mutereason, a3.mutedby, a1.failed_logins, a1.locked, a1.os FROM account AS a1 LEFT JOIN account_access AS a2 ON (a1.id = a2.id AND (a2.RealmID = ? OR a2.RealmID = -1)) LEFT JOIN account_muted AS a3 ON (a1.id = a3.account AND (a3.active = 1)) WHERE a1.id = ?", CONNECTION_SYNCH);
Copy link
Member

Choose a reason for hiding this comment

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

it's kinda unreadable with account AS a1, account_access AS a2 and account_muted AS a3

Copy link
Author

Choose a reason for hiding this comment

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

what do you suggest ?

Copy link
Member

Choose a reason for hiding this comment

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

something like account AS a, account_access AS aa, account_muted AS am.
I'd use the full table name but it seems to be inconsistent with existing SQL queries.

Copy link
Member

Choose a reason for hiding this comment

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

and you really don't need the AS keyword

@ghost
Copy link
Author

ghost commented Dec 17, 2015

@jackpoz
@Shauren
Looks fine now

@jackpoz jackpoz self-assigned this Dec 17, 2015
@jackpoz
Copy link
Member

jackpoz commented Dec 19, 2015

let's say I mute a player for 10 minutes and right after I do that I mute him again for 60 minutes. Is there a chance in the current code behavior compared to this PR ? Is the player gonna get unmuted after 10 or 60 minutes ?

I'm also wondering about the performance comparision between current structure and the PR one, adding a join to a table that will get huge over time.

@jackpoz
Copy link
Member

jackpoz commented Dec 19, 2015

The idea behind mute history implemented by 15a9c67 is to log who was muted when why and by who, a server admin could even choose to flush the table every once in a while. Moving the handling of mutes to that table might not be a good idea, it wasn't designed to do that.

@ghost
Copy link
Author

ghost commented Dec 19, 2015

@jackpoz
After what you said I saw that PR needs to be completely reworked.
I'll work on it and I will open another PR when possible, thanks everyone.

@jackpoz
Copy link
Member

jackpoz commented Dec 19, 2015

Before working on it wait till we figure out if we want to move the data to account_muted or if we prefer to leave it the way it is, we don't want you to waste your time

@jackpoz
Copy link
Member

jackpoz commented Jan 3, 2016

@mthsena after considering how the account_muted table was added and how it works, I think it's better to leave the current code the way it is. There's little to gain with the new structure and many possible headaches on the horizon, modifying code that currently works very well.
Maybe if we noticed it before you wouldn't have gone this far with all the troubles of this PR. Thank you for the contribution tho!

@jackpoz jackpoz closed this Jan 3, 2016
@ghost
Copy link
Author

ghost commented Jan 3, 2016

Yes, thank you for your efforts, @mthsena , even if it did not get completed. I hope you can make use of your experiences with this PR to start again on some other task of your choice. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jackpoz jackpoz

Labels
Branch-3.3.5a Comp-Core Feedback-PatchFix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jackpoz @MitchesD @Aokromes @Shauren @Naios @mthsena