return correct uncharged item when charges run out

[nwjgit]

closes: #5305

Description:

This addresses the issue of charged items reverting to their normal variant when minion charges reach zero. i.e. when you have a Sanguine Scythe of vitur and you run out of charges you are given a Scythe of vitur (uncharged) and the kit is lost.

Changes:

Changes how the bot determines which item to give back to the user. This returns the correct unchanged version of the item that has reached zero charges.

Other checks:

• I have tested all my changes thoroughly.

I have done extensive testing in my private server. It may be beneficial to move this over to the test server and let the community do more testing to ensure no bugs.

[themrrobert]

Too many problems as-is.

• the name run is too generic, it should be refundOnBreak
• The code is bugged, and won't remove the degraded item unless it also refunds an item
• The code doesn't take into consideration items degraded from the bank
• A union type should be used to avoid creating anonymous functions to return a constant value
• The constant Bank()'s should end with .freeze(), which i know isn't your bug, but this would be a good place to fix that.
• I don't think anonymous functions are the right solution here.... it should probably be some kind of refundVariants array which is easier to maintain:

Something like:

    refundVariants: [ ['Holy scythe', 'Holy scythe (uncharged)' ], [ 'Sang scythe', 'Sang scythe uncharged'] ];

or maybe even

    refundVariants: [
        {
            variant: getOSItem('Holy scythe of vitur'),
            refund: new Bank().add('Holy scythe of vitur (uncharged')
        }, {
            variant: getOSItem('Sanguine scythe of vitur'),
            refund: new Bank().add('Sanguine scythe of vitur (uncharged)').freeze();
        }
    ],

And then the degradeItem() logic will obviously have to be updated to support the chosen method.

[themrrobert]

This is where the bug I mentioned is; notice it only removes the degraded item if there's an item to refund.

This may not always be the case if something degrades into dust, and should be accounted for, even if not used currently.

[themrrobert]

There is no reason to use anonymous arrays here; these should remain constant Bank() objects, and a union type + type checking should be used to support both types.

[nwjgit]

Thanks for all the feedback, I believe I made all the adjustments you wanted.

Something that may need addressed is the "unchargedItem" not taking into account variants when trying to charge items. Currently it isn't an issue because the only items that have variants are scythe & sang staff . Both have /create to make their charged versions and then /minion charge works without error. But this won't work if for example the shadow gets a variant since it's charged version isn't under /create.

I imagine something could be done with /degradableItemsCommand similar to what I've done here. Let me know if that should be addressed in a new pr or you would like me to add it to this one.

[nwjgit]

Could this be reviewed again? People are still losing their kits.