VC Activity command crash fix #300 #343

Budbomber · 2022-01-14T13:40:48Z

Set a max duration of a week in seconds, (Not sure what the best way to do it was, so went with a 606024) 7 days in seconds.

Added required range to MAX_AGE_OPTION (0, MAX_AGE_DURATION_ONE_WEEK).

…imum. Added: .setRequiredRange(1, TimeUnit.DAYS.toSeconds(MAX_AGE_DURATION))); sets required range between 1 and 7 days.

…n seconds. Added to option MAX_AGE_OPTION: .setRequiredRange(1,NAX_DURATION_ONE_WEEK)); sets required range between 1 and 7 days in seconds

CLAassistant · 2022-01-14T13:40:57Z

All committers have signed the CLA.

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

…c/VcActivityCommand.java Changed, MAX_DURATION_ONE_WEEK to more readable, MAX_VALUE_MAX_AGE, and used ChronoUnit to convert seconds to days. Co-authored-by: Tais993 <[email protected]>

Zabuzard · 2022-01-14T13:56:19Z

Apart from what has been mentioned, looks fine. That said, what about the rest of the code? If I remember correctly, it had some check (handleIntegerOption or sth like that) which might now be (partially?) obsolete?

Please double check and if necessary clean up, thanks.

…c/VcActivityCommand.java More readable, removed irrelevant comment Co-authored-by: Tais993 <[email protected]>

Budbomber

More readable following changes, accepted suggested.

Budbomber · 2022-01-14T14:00:20Z

@Zabuzard

This would also mean changing max users to the same effect as age, and removing that handler totally, (I think)

Zabuzard · 2022-01-14T14:01:19Z

Set a max duration of a week in seconds, (Not sure what the best way to do it was, so went with a 60_60_24) 7 days in seconds.
Added required range to MAX_AGE_OPTION (0, MAX_AGE_DURATION_ONE_WEEK).

This would also mean changing max users to the same effect as age, and the removing that handler totally

Thats fair, but what about this part of the code:

Is it really still necessary like that, or can it be simplified now?

Budbomber · 2022-01-14T14:02:11Z

Set a max duration of a week in seconds, (Not sure what the best way to do it was, so went with a 60_60_24) 7 days in seconds.
Added required range to MAX_AGE_OPTION (0, MAX_AGE_DURATION_ONE_WEEK).

This would also mean changing max users to the same effect as age, and the removing that handler totally

Thats fair, but what about this part of the code: Is it really still necessary like that, or can it be simplified now?

Pretty sure we can just remove that, plus the calls to the error handlers

Tais993 · 2022-01-14T14:03:07Z

I see that there also is a max to the max-uses option, see the Discord docs here

Has to be between 0 and 100, fix that too in this PR?

That way the handleIntegerTypeOption's validation regarding it being a valid Integer can be removed

Budbomber · 2022-01-14T14:03:59Z

No worries, I'll work on that now and clean up it up

…n seconds. Added to option MAX_AGE_OPTION: .setRequiredRange(1,NAX_DURATION_ONE_WEEK)); sets required range between 1 and 7 days in seconds

# Conflicts: # application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

…LUE to more readable TimeUnit

…AGE to more readable TimeUnit.

Budbomber · 2022-01-14T15:46:46Z

No worries, I'll work on that now and clean up it up

Seems to be working now, changes made, any input appreciated

Budbomber

Perfect

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

…c/VcActivityCommand.java wrong values (oops) Co-authored-by: Jonas <[email protected]>

illuminator3

LGTM

Tais993

I personally don't like all the new lines you added, it's too many of them imo

Besides that, it looks good

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Budbomber · 2022-01-15T22:22:29Z

Yeah sorry bud must of been the merges etc I failed haha next PR I will do much neater haha

illuminator3

Approving this as Tijs has also approved it.

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

Zabuzard · 2022-01-17T08:06:58Z

@Budbomber Please dont resolve conversations yourself. Let them open and wait until the person who requested change approves and closes them, thanks.

…removed some more empty lines etc.

… doc updated. other format changes.

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

…l createInvite for maxAgeDays if null default set otherwise user input set.

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

… MAX_AGE_LIMIT & MAX_USES_LIMIT

…text will display correctly.

Tais993

Budbomber added 2 commits January 13, 2022 20:12

Added: MAX_AGE_DURATION - This converts seconds to days (7) Being max…

8fbb2c2

…imum. Added: .setRequiredRange(1, TimeUnit.DAYS.toSeconds(MAX_AGE_DURATION))); sets required range between 1 and 7 days.

Added: MAX_AGE_DURATION_ONE_WEEK sets the duration the max duration i…

3e718a7

…n seconds. Added to option MAX_AGE_OPTION: .setRequiredRange(1,NAX_DURATION_ONE_WEEK)); sets required range between 1 and 7 days in seconds

Budbomber requested review from a team as code owners January 14, 2022 13:40

Tais993 linked an issue Jan 14, 2022 that may be closed by this pull request

VC Activity command crashes #300

Closed

Tais993 added bug Something isn't working priority: normal labels Jan 14, 2022

Tais993 assigned Budbomber Jan 14, 2022

Tais993 added this to the Improvement phase 1 milestone Jan 14, 2022

Tais993 requested changes Jan 14, 2022

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

903fcf6

…c/VcActivityCommand.java Changed, MAX_DURATION_ONE_WEEK to more readable, MAX_VALUE_MAX_AGE, and used ChronoUnit to convert seconds to days. Co-authored-by: Tais993 <[email protected]>

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

2e97eda

…c/VcActivityCommand.java More readable, removed irrelevant comment Co-authored-by: Tais993 <[email protected]>

Budbomber commented Jan 14, 2022

View reviewed changes

Budbomber added 4 commits January 14, 2022 15:26

Added: MAX_AGE_DURATION_ONE_WEEK sets the duration the max duration i…

ff8f11c

…n seconds. Added to option MAX_AGE_OPTION: .setRequiredRange(1,NAX_DURATION_ONE_WEEK)); sets required range between 1 and 7 days in seconds

Merge remote-tracking branch 'origin/develop' into develop

0756745

# Conflicts: # application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java

Cleaned up code, Added MAX_USES_MAX VALUE, reformatted MAX_AGE_MAX_VA…

9ccf518

…LUE to more readable TimeUnit

Cleaned up code, Added MAX_VALUE_MAX_USES, reformatted MAX_VALUE_MAX_…

4d015aa

…AGE to more readable TimeUnit.

Budbomber requested a review from Tais993 January 14, 2022 15:49

Budbomber commented Jan 14, 2022

View reviewed changes

illuminator3 requested changes Jan 14, 2022

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

93b9927

…c/VcActivityCommand.java wrong values (oops) Co-authored-by: Jonas <[email protected]>

illuminator3 previously approved these changes Jan 14, 2022

View reviewed changes

Tais993 reviewed Jan 15, 2022

View reviewed changes

Budbomber and others added 6 commits January 15, 2022 22:20

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

f7d0640

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

a49095c

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

4ddbd6c

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

29d217f

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

a4ccedb

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Update application/src/main/java/org/togetherjava/tjbot/commands/basi…

aeb8e79

…c/VcActivityCommand.java Co-authored-by: Tais993 <[email protected]>

Tais993 previously approved these changes Jan 15, 2022

View reviewed changes

illuminator3 previously approved these changes Jan 16, 2022

View reviewed changes

illuminator3 enabled auto-merge (squash) January 16, 2022 02:19

Zabuzard requested changes Jan 17, 2022

View reviewed changes

Made changes Zabu requested, typos, Moved declarations, updated doc, …

97ed98a

…removed some more empty lines etc.

auto-merge was automatically disabled January 17, 2022 10:21
Head branch was pushed to by a user without write access

Budbomber dismissed stale reviews from illuminator3 and Tais993 via 97ed98a January 17, 2022 10:21

Final changes made, renamed handle method to RequiredOptionIfPresent,…

0aca975

… doc updated. other format changes.

Zabuzard requested changes Jan 17, 2022

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

Changes made, remaned maxAge to maxAgeDays, added check in voicechane…

c11b8ea

…l createInvite for maxAgeDays if null default set otherwise user input set.

Zabuzard requested changes Jan 17, 2022

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

application/src/main/java/org/togetherjava/tjbot/commands/basic/VcActivityCommand.java Outdated Show resolved Hide resolved

Fixed typo's changed text in options to reflect the values set in the…

19062bc

… MAX_AGE_LIMIT & MAX_USES_LIMIT

Zabuzard previously approved these changes Jan 17, 2022

View reviewed changes

Fixed value that was forgotten, input is now in days for UX, and the …

0a98196

…text will display correctly.

Budbomber dismissed Zabuzard’s stale review via 0a98196 January 17, 2022 14:31

Zabuzard approved these changes Jan 17, 2022

View reviewed changes

Budbomber requested a review from Tais993 January 17, 2022 14:46

Tais993 approved these changes Jan 18, 2022

View reviewed changes

Zabuzard enabled auto-merge (squash) January 18, 2022 13:06

Zabuzard merged commit 0be28de into Together-Java:develop Jan 18, 2022

Uh oh!

VC Activity command crash fix #300 #343

VC Activity command crash fix #300 #343

Uh oh!

Conversation

Budbomber commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zabuzard commented Jan 14, 2022

Uh oh!

Budbomber left a comment

Choose a reason for hiding this comment

Uh oh!

Budbomber commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zabuzard commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Budbomber commented Jan 14, 2022

Uh oh!

Tais993 commented Jan 14, 2022

Uh oh!

Budbomber commented Jan 14, 2022

Uh oh!

Budbomber commented Jan 14, 2022

Uh oh!

Budbomber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

illuminator3 left a comment

Choose a reason for hiding this comment

Uh oh!

Tais993 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Budbomber commented Jan 15, 2022

Uh oh!

illuminator3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zabuzard commented Jan 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tais993 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Budbomber commented Jan 14, 2022 •

edited

Loading

CLAassistant commented Jan 14, 2022 •

edited

Loading

Budbomber commented Jan 14, 2022 •

edited

Loading

Zabuzard commented Jan 14, 2022 •

edited

Loading