Alter Account & Transaction and fix TownyTransactionEvent. #7431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LlmDl
changed the title
Warriorrrr
reviewed
May 26, 2024
Towny/src/main/java/com/palmergames/bukkit/towny/object/economy/Account.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/towny/object/Transaction.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/towny/TownyEconomyHandler.java
Outdated
Show resolved
Hide resolved
LlmDl
changed the title
LlmDl
force-pushed
the
fix/TownyTransactionEvent
branch
from
d4c9ca8 to
bfc77f7
Compare
Alters Transactions to no longer use a Player and instead try to use a TownyObject. Rethink Transaction to adopt a Account instead of a TownyObject and Name. Fix javadoc Fix Account initializing TownyObject null. Add helper method to Transaction. Clean up EconomyAccount, move the TownyServerAccount instance to Account. Alter Transaction again in order to add both Receiving and Sending accounts. Remove getName/getTownyObject from Transaction, change TownyObject in Account over to EconomyHandler. Return government field to BankAccount. Throw the TownyTransactionEvent earlier, so that we can better support the new sender and receiver fields in the Transaction. Fix flipped sender/receiver. Notify observers of payTo/payFromServer only when the transaction is a success. A very hacky way to get the TownyServerAccount to implement EconomyHandler. Change transaction package, adopt new builder pattern, deprecate existing Transaction and TransactionType classes. Reduce diff and a couple better javadocs
annotations and helper methods to Transaction.
LlmDl
force-pushed
the
fix/TownyTransactionEvent
branch
from
bfc77f7 to
e88ba0c
Compare
Warriorrrr
reviewed
Jul 12, 2024
Towny/src/main/java/com/palmergames/bukkit/towny/object/economy/transaction/Transaction.java
Outdated
Show resolved
Hide resolved
Towny/src/main/java/com/palmergames/bukkit/towny/object/economy/transaction/Transaction.java
Outdated
Show resolved
Hide resolved
Comment on lines
+15
to
+20
| public Transaction(TransactionBuilder builder) { | ||
| this.type = builder.type; | ||
| this.receivingAccount = builder.receivingAccount; | ||
| this.sendingAccount = builder.sendingAccount; | ||
| this.amount = builder.amount; | ||
| } |
There was a problem hiding this comment.
I would make this a normal constructor instead of requiring the use of the builder
Suggested change
| public Transaction(TransactionBuilder builder) { | |
| this.type = builder.type; | |
| this.receivingAccount = builder.receivingAccount; | |
| this.sendingAccount = builder.sendingAccount; | |
| this.amount = builder.amount; | |
| } | |
| @Contract("_, null, null, _ -> fail") | |
| public Transaction(@NotNull TransactionType type, Account receivingAccount, Account sendingAccount, double amount) { | |
| Preconditions.checkNotNull(type, "type"); | |
| Preconditions.checkState(receivingAccount != null || sendingAccount != null, "receivingAccount and sendingAccount cannot both be null"); | |
| Preconditions.checkState(amount > 0, "amount must be greater than 0"); | |
| this.type = type; | |
| this.receivingAccount = receivingAccount; | |
| this.sendingAccount = sendingAccount; | |
| this.amount = amount; | |
| } |
Warriorrrr
approved these changes
Jul 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Deprecates the original Transaction and creates a new Transaction class in its own package within the
object.economypackage.The new Transaction uses a amount, TransactionType and two accounts (sender and receiver.) Transactions are made using a factory pattern which makes it clearer who is paying and who is being paid. This builder can create Transactions as well as TownyTransactionEvents (for which Transactions are usually being used.)
Additionally, this PR makes it so that TownyTransactionEvents are thrown much more frequently, rather than only under very limited circumstances.
Additionally, this PR adds an EconomyHandler to the Account class, reducing the importance of an Account's Name field.
Additionally, this PR alters the TownyServerAccount to now also implement EconomyHandler so that it can behave correctly with the new Account change.
Should make things easier to switch from our use of
Account.getName()toAccount.getUUID()when #7425 is ready to merge.Post Testing, found the following plugins require updating:
New Nodes/Commands/ConfigOptions:
Relevant Towny Issue ticket:
By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the Towny License for perpetuity.