Create use effect command #419

Open
Vylpes wants to merge 23 commits from feature/380-use-effect into develop
Owner

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

#380

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify the changes. Provide instructions so we can reproduce. Please also list any relevant details to your test configuration.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that provde my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
# Description Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. #380 ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update # How Has This Been Tested? Please describe the tests that you ran to verify the changes. Provide instructions so we can reproduce. Please also list any relevant details to your test configuration. # Checklist - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that provde my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules
Vylpes added 10 commits 2024-12-31 16:12:01 +00:00
Vylpes added 2 commits 2024-12-31 17:00:40 +00:00
Fix linting issues
All checks were successful
Test / build (push) Successful in 28s
222d990a31
Vylpes added the
type
story
label 2024-12-31 17:08:59 +00:00
Vylpes requested review from Copilot 2024-12-31 17:09:40 +00:00
Copilot reviewed 2024-12-31 17:12:13 +00:00
@ -33,0 +56,4 @@
const effectDetail = EffectDetails.get(id);
if (!effectDetail) {
await interaction.reply("Unable to find effect!");
Member

The error message 'Unable to find effect!' is unclear. It should be 'Effect not found in the system!'.

The error message 'Unable to find effect!' is unclear. It should be 'Effect not found in the system!'.
Vylpes marked this conversation as resolved
@ -33,0 +114,4 @@
const effectDetail = EffectDetails.get(id);
if (!effectDetail) {
await interaction.reply("Unable to find effect!");
Member

The error message 'Unable to find effect!' is unclear. It should be 'Effect not found in the system!'.

The error message 'Unable to find effect!' is unclear. It should be 'Effect not found in the system!'.
Vylpes marked this conversation as resolved
Copilot refused to review 2024-12-31 17:12:19 +00:00
Vylpes changed title from WIP: Create use effect command to Create use effect command 2024-12-31 17:12:48 +00:00
Vylpes added 1 commit 2025-01-03 14:31:41 +00:00
Fix suggested changes
All checks were successful
Test / build (push) Successful in 27s
ff2980f3c0
Vylpes requested review from Copilot 2025-01-03 14:33:32 +00:00
Copilot reviewed 2025-01-03 14:37:48 +00:00
Copilot left a comment
Member

Copilot reviewed 6 out of 16 changed files in this pull request and generated 2 comments.

Files not reviewed (10)
  • .env.example: Language not supported
  • tests/commands/snapshots/effects.test.ts.snap: Language not supported
  • tests/helpers/snapshots/EffectHelper.test.ts.snap: Language not supported
  • tests/commands/effects.test.ts: Evaluated as low risk
  • tests/database/entities/app/UserEffect.test.ts: Evaluated as low risk
  • tests/helpers/EffectHelper.test.ts: Evaluated as low risk
  • src/commands/effects.ts: Evaluated as low risk
  • src/constants/CardConstants.ts: Evaluated as low risk
  • src/constants/EffectDetails.ts: Evaluated as low risk
  • src/helpers/CardDropHelperMetadata.ts: Evaluated as low risk
Comments suppressed due to low confidence (8)

tests/buttonEvents/Effects.test.ts:42

  • [nitpick] The error message 'Page option is not a valid number' is hardcoded in multiple places. Consider extracting it into a constant.
expect(interaction.reply).toHaveBeenCalledWith("Page option is not a valid number");

tests/buttonEvents/Effects.test.ts:83

  • [nitpick] The error message 'Unable to find effect!' is hardcoded in multiple places. Consider extracting it into a constant.
expect(interaction.reply).toHaveBeenCalledWith("Unable to find effect!");

tests/buttonEvents/Effects.test.ts:90

  • [nitpick] The error message 'Unable to find effect!' is hardcoded in multiple places. Consider extracting it into a constant.
expect(interaction.reply).toHaveBeenCalledWith("Unable to find effect!");

tests/buttonEvents/Effects.test.ts:117

  • [nitpick] The error message 'Unable to use effect! Please make sure you have it in your inventory and is not on cooldown' is hardcoded. Consider extracting it into a constant.
expect(interaction.reply).toHaveBeenCalledWith("Unable to use effect! Please make sure you have it in your inventory and is not on cooldown");

tests/buttonEvents/Effects.test.ts:81

  • Ensure that interaction.reply is called only once in the error scenario where the effect detail is not found.
await effects.UseConfirm(interaction);

tests/buttonEvents/Effects.test.ts:88

  • Ensure that interaction.reply is called only once in the error scenario where the effect detail is not found.
await effects.UseCancel(interaction);

src/helpers/TimeLengthInput.ts:122

  • Consider adding tests to cover the new ConvertFromMilliseconds method to ensure it works correctly for various input values.
public static ConvertFromMilliseconds(ms: number): TimeLengthInput {

src/helpers/TimeLengthInput.ts:132

  • [nitpick] Ensure that the timeString format is consistent with the rest of the codebase and meets the requirements for all use cases.
const timeString = `${days}d ${remainingHours}h ${remainingMinutes}m ${remainingSeconds}s`;
Copilot reviewed 6 out of 16 changed files in this pull request and generated 2 comments. <details> <summary>Files not reviewed (10)</summary> * **.env.example**: Language not supported * **tests/commands/__snapshots__/effects.test.ts.snap**: Language not supported * **tests/helpers/__snapshots__/EffectHelper.test.ts.snap**: Language not supported * **tests/commands/effects.test.ts**: Evaluated as low risk * **tests/database/entities/app/UserEffect.test.ts**: Evaluated as low risk * **tests/helpers/EffectHelper.test.ts**: Evaluated as low risk * **src/commands/effects.ts**: Evaluated as low risk * **src/constants/CardConstants.ts**: Evaluated as low risk * **src/constants/EffectDetails.ts**: Evaluated as low risk * **src/helpers/CardDropHelperMetadata.ts**: Evaluated as low risk </details> <details> <summary>Comments suppressed due to low confidence (8)</summary> **tests/buttonEvents/Effects.test.ts:42** * [nitpick] The error message 'Page option is not a valid number' is hardcoded in multiple places. Consider extracting it into a constant. ``` expect(interaction.reply).toHaveBeenCalledWith("Page option is not a valid number"); ``` **tests/buttonEvents/Effects.test.ts:83** * [nitpick] The error message 'Unable to find effect!' is hardcoded in multiple places. Consider extracting it into a constant. ``` expect(interaction.reply).toHaveBeenCalledWith("Unable to find effect!"); ``` **tests/buttonEvents/Effects.test.ts:90** * [nitpick] The error message 'Unable to find effect!' is hardcoded in multiple places. Consider extracting it into a constant. ``` expect(interaction.reply).toHaveBeenCalledWith("Unable to find effect!"); ``` **tests/buttonEvents/Effects.test.ts:117** * [nitpick] The error message 'Unable to use effect! Please make sure you have it in your inventory and is not on cooldown' is hardcoded. Consider extracting it into a constant. ``` expect(interaction.reply).toHaveBeenCalledWith("Unable to use effect! Please make sure you have it in your inventory and is not on cooldown"); ``` **tests/buttonEvents/Effects.test.ts:81** * Ensure that `interaction.reply` is called only once in the error scenario where the effect detail is not found. ``` await effects.UseConfirm(interaction); ``` **tests/buttonEvents/Effects.test.ts:88** * Ensure that `interaction.reply` is called only once in the error scenario where the effect detail is not found. ``` await effects.UseCancel(interaction); ``` **src/helpers/TimeLengthInput.ts:122** * Consider adding tests to cover the new ConvertFromMilliseconds method to ensure it works correctly for various input values. ``` public static ConvertFromMilliseconds(ms: number): TimeLengthInput { ``` **src/helpers/TimeLengthInput.ts:132** * [nitpick] Ensure that the timeString format is consistent with the rest of the codebase and meets the requirements for all use cases. ``` const timeString = `${days}d ${remainingHours}h ${remainingMinutes}m ${remainingSeconds}s`; ``` </details>
@ -61,0 +99,4 @@
const randomCardIndex = Math.floor(Math.random() * allCards.length);
const card = allCards[randomCardIndex];
Member

The variable 'card' can be undefined if 'allCards' is empty. Add a check to ensure 'allCards' is not empty before accessing an element by index.

- const card = allCards[randomCardIndex];
+ if (allCards.length === 0) return undefined;
The variable 'card' can be undefined if 'allCards' is empty. Add a check to ensure 'allCards' is not empty before accessing an element by index. ```diff - const card = allCards[randomCardIndex]; + if (allCards.length === 0) return undefined; ```
Vylpes marked this conversation as resolved
Copilot refused to review 2025-01-03 14:38:37 +00:00
Vylpes added 1 commit 2025-01-03 14:41:16 +00:00
Fix undefined error if allCards variable is null
All checks were successful
Test / build (push) Successful in 26s
0092d91ee6
Vylpes requested review from VylpesTester 2025-01-03 14:43:46 +00:00
VylpesTester was assigned by Vylpes 2025-01-03 14:43:49 +00:00
VylpesTester requested changes 2025-01-08 17:42:12 +00:00
@ -1,6 +1,9 @@
import {ButtonInteraction} from "discord.js";
import {ActionRowBuilder, ButtonBuilder, ButtonInteraction, ButtonStyle, EmbedBuilder} from "discord.js";
Member

Fix spacing

Fix spacing
Vylpes marked this conversation as resolved
@ -14,3 +20,3 @@
}
private async List(interaction: ButtonInteraction) {
public async List(interaction: ButtonInteraction) {
Member

I don't like that

I don't like that
Vylpes marked this conversation as resolved
@ -31,2 +37,4 @@
});
}
public async Use(interaction: ButtonInteraction) {
Member

I don't like that its public

I don't like that its public
Vylpes marked this conversation as resolved
@ -33,0 +50,4 @@
}
}
public async UseConfirm(interaction: ButtonInteraction) {
Member

I don't like that thats public

I don't like that thats public
Vylpes marked this conversation as resolved
@ -33,0 +108,4 @@
await interaction.reply("Unable to use effect! Please make sure you have it in your inventory and is not on cooldown");
}
public async UseCancel(interaction: ButtonInteraction) {
Member

I don't like that thats public

I don't like that thats public
Vylpes marked this conversation as resolved
@ -1,6 +1,9 @@
import {CommandInteraction, SlashCommandBuilder} from "discord.js";
import {ActionRowBuilder, ButtonBuilder, ButtonStyle, CommandInteraction, EmbedBuilder, SlashCommandBuilder} from "discord.js";
Member

Spacing

Spacing
Vylpes marked this conversation as resolved
@ -45,0 +65,4 @@
const effectDetail = EffectDetails.get(id);
if (!effectDetail) {
await interaction.reply("Unable to find effect!");
Member

I think we should log a warning in this case, since it shouldn't be possible

I think we should log a warning in this case, since it shouldn't be possible
Vylpes marked this conversation as resolved
@ -9,1 +9,4 @@
public static readonly MultidropQuantity = 11;
// Effects
public static readonly UnusedChanceUpChance = 1;
Member

Incorrect value

Incorrect value
Vylpes marked this conversation as resolved
@ -58,6 +59,66 @@ export default class CardDropHelperMetadata {
};
}
public static async GetRandomCardUnclaimed(userId: string): Promise<DropResult | undefined> {
Member

I think we should split this file up now

I think we should split this file up now
Vylpes marked this conversation as resolved
@ -1,6 +1,7 @@
import {ActionRowBuilder, ButtonBuilder, ButtonStyle, EmbedBuilder} from "discord.js";
import UserEffect from "../database/entities/app/UserEffect";
import EmbedColours from "../constants/EmbedColours";
import {EffectDetails} from "../constants/EffectDetails";
Member

Spacing

Spacing
Vylpes marked this conversation as resolved
@ -119,2 +119,4 @@
return desNumber;
}
public static ConvertFromMilliseconds(ms: number): TimeLengthInput {
Member

I want tests generated for this

I want tests generated for this
Vylpes marked this conversation as resolved
@ -1,127 +1,135 @@
import {ButtonInteraction} from "discord.js";
Member

Recreate the tests

Recreate the tests
Vylpes marked this conversation as resolved
Vylpes added 1 commit 2025-01-10 18:23:20 +00:00
WIP: Fix some of the suggested changes
Some checks failed
Test / build (push) Failing after 23s
31cc9e056a
Vylpes added 1 commit 2025-01-11 18:58:38 +00:00
WIP: Plan tests
All checks were successful
Test / build (push) Successful in 30s
d794b30bb5
Vylpes added 1 commit 2025-01-13 17:57:25 +00:00
WIP: EFfects List Button Event tests
All checks were successful
Test / build (push) Successful in 30s
6c0b7c6899
Vylpes added 1 commit 2025-01-16 20:34:36 +00:00
WIP: Create UseEffect tests
Some checks failed
Test / build (push) Failing after 32s
3d665aba67
Vylpes started working 2025-01-17 21:07:47 +00:00
Vylpes stopped working 2025-01-17 21:44:21 +00:00
36 minutes 34 seconds
Vylpes added 1 commit 2025-01-18 16:49:53 +00:00
WIP: Start of Claim tests
Some checks failed
Test / build (push) Failing after 33s
6e6ad3331a
Vylpes added 4 commits 2025-01-22 18:08:10 +00:00
Vylpes requested review from VylpesTester 2025-01-22 18:08:56 +00:00
All checks were successful
Test / build (push) Successful in 34s
Required
Details
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/380-use-effect:feature/380-use-effect
git checkout feature/380-use-effect
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Total time spent: 36 minutes 34 seconds
Vylpes
36 minutes 34 seconds
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: External/card-drop#419
No description provided.