-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
refactor(Core/ServerMail): Refactor to Dedicated Manager Class with Multi-Item & Condition Support #21590
refactor(Core/ServerMail): Refactor to Dedicated Manager Class with Multi-Item & Condition Support #21590
Conversation
* Allow ServerMail to send multiple items by introducing new DB table mail_server_template_items * Merge existing entries from mail_server_template to mail_server_template_items before dropping columns * Add foreign keys to mail_server_character.mailId and mail_server_template_items.templateID to remove entries if the parent mail_server_template.id is removed * Clean up and add early return for ObjectMgr::SendServerMail * closes azerothcore#11446
data/sql/updates/pending_db_characters/rev_1740310084140595600.sql
Outdated
Show resolved
Hide resolved
…nu/azerothcore-wotlk into mail_server_template_items
I think this PR is done. I have updated the description with in-depth detail of what has changed |
continue; | ||
} | ||
|
||
ServerMailConditionType conditionType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not store as the type (uint8) in the database rather than string and having to manually convert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to storing it as an Enum in the database for easy readability. But if that is overkill we can swap it, sure
_serverMailStore[templateID].itemsH.push_back(mailItem); | ||
else | ||
{ | ||
LOG_ERROR("sql.sql", "Table `mail_server_template_items` has invalid faction value '{}' for templateID {}, skipped.", faction, templateID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo it's best to do the validation above where the rest of the validations are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont really understand? We need it after the other validation to know where to push it.
Also the else
statement should in reality never be able to hit since we use enum in the database for faction
I have addressed most reviews, thanks for them! |
Changes Proposed:
ObjectMgr
into a new dedicatedServerMailMgr
classmail_server_template_items
tablemail_server_template_conditions
tablemail_server_character
,mail_server_template_conditions
,mail_server_template_items
foreign key with on delete cascade is added for automatic removal of entries if mail_server_template.id is deleted.mail_server_template
itemA
,itemH
,itemCountA
,itemCountH
,reqLevel
,reqPlayTime
mail_server_template_items
mail_server_template_conditions
mail_server_character
mail_server_template_conditions
Migration queries (included in the SQL script)
This PR proposes changes to:
Issues Addressed:
Tests Performed:
This PR has been:
How to Test the Changes:
Warning
When testing, remove the SQL file from the updates folder. Otherwise, if you restart your server, the auto updater will run it again which will result in errors.
How to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.