TJ-Bot first issue

[cobalt ruin]

trynna work with an issue on TJ-Bot, have a few questions.

[woven hull]

<@&987246924425994290> please have a look, thanks.

[woven hull]

While you are waiting for getting help, here are some tips to improve your experience:

Code is much easier to read if posted with syntax highlighting and proper formatting.

If nobody is calling back, that usually means that your question was not well asked and hence nobody feels confident enough answering. Try to use your time to elaborate, provide details, context, more code, examples and maybe some screenshots. With enough info, someone knows the answer for sure.

Don't forget to close your thread using the command </help-thread close:1027500463647621170> when your question has been answered, thanks.

[cobalt ruin]

this is the one im talking about

i have already created an sql file to add a column to reminder table as suggested. Im not sure if it should have a default value or not

this is what it look like atm

don't feel like it should have a default value tho, since the column contains no of times the /reminder command failed basically

Another question

Zabu mentioned RemindRoutine.java is responsible for sending reminders

im assuming this method is the one that handles that part inside RemindRoutine.java file

[drifting oar]

depending on the design, the default could be 0 (retryAttempts) or 3 (retryAttemptsLeft)

[drifting oar]

correct. that sendReminderViaRoute does it

it figures out where to send it to (channel or DM) and then sends it there with that method

[cobalt ruin]

so sendReminderViaRoute mainly does the job of sending that message back to db

[drifting oar]

yes

[cobalt ruin]

any ideas how should i go about it? try to wrap it in try-catch block or use some sort of conditional along with boolean

don't think it would throw an exception if it misses the call

[drifting oar]

its a restaction

when u do .queue(...) it has a failure callback there

as second argument

that way u can react to failures

[cobalt ruin]

ah, i'll try something out see how it goes

[drifting oar]

for testing, just throw an exception within the reminder and it triggers lol

[cobalt ruin]

where were we supposed to put our bot token 👀

from what i remember someone suggested earlier to do that in a file i think

was wondering if there's a better way or more standard

[drifting oar]

in tjbot it's read via the config file

[cobalt ruin]

apart from putting token in config.json.template file, is there anthying else i need to add? tried doing a run but test-bot in my server seems ded

[drifting oar]

the template file is just a template

ur supposed to create the actual config. u can use the template for copy pasta

but the bot will read the actual config.json

not a template

by default the config file is autocreated on first launch based on the template in the working directory

u can probably find it somewhere there

alternatively, the bot accepts the first parameter in main (run config) as path to the config file

u can check the wiki, those config steps should be covered there - with images

[cobalt ruin]

oh lol, finally got it running

is this normal? the bot seems active on server tho

[cobalt ruin]

oh thanks, it shouldnt affect the functionality then

[drifting oar]

its an optional feature. hence it's a warning, not an error

[drifting oar]

it actually has an explanation text right above it iirc

or below

https://i.imgur.com/PlahlG7.png

i guess it could be adjusted to sth like "... You can ignore this warning if you are not interested in this feature"

[cobalt ruin]

write is used to write to db, but i don't understand if this snippet is used to send reminder from db to user or saving it to db

[drifting oar]

sending from DB to user

saving to DB is done in the slash command

the code u shared does:

• get hands on all entries in the DB
• filter by reminders that due to now (remind at < now)
• for each of them
  • sendReminder (to user)
  • delete it from DB

[cobalt ruin]

oh since we are deleteing entries that's why we chose the write command

otherwise read would have been enough

as for my logic to increment the counter and retry should fit here?

[silent ice]

yes, but it would be better to put it in a separate method

also why do you extract the consumer parameter?

[drifting oar]

that was done by me

its the current code

but yeah. it will become complex enough to justify its own method at that point

but yes, it would go inside there

the idea is that (as the original entry is deleted from the DB), u reinsert it. with increased timer and and instant that is overdue as well - or scheduled in like 1 min

so that the next routine-run will naturally pick it up

[silent ice]

i was thinking of maybe a for loop

which runs 3 times for now

and in every iteration we add an onFailure call to the completable future, and assign that to the old variable

or we could use rest action too if possible

would make it easy if we were to increase the number of attempts

[drifting oar]

dont do that, then u would schedule immediately again

the idea is to wait for a few seconds or minutes

cause failure usually is caused by network issues or rate limits

if u retry immediately, chances are high that it fails again

i feel like its easier to just use the existing routine-run system

i.e. just reinserting into the DB and it will naturally be executed again

that way its just like 4 lines of code or so

[cobalt ruin]

ok thanks for the direction, ill try to implement this in a separate method. Is there a way to replicate the fail while using the command.

[silent ice]

so use a sheduled exectuor inside the rest action?

or u can use RestAction#queueAfter

[drifting oar]

no. just db.insert(reminder(...remindAt.plusMinutes(1), retry + 1)

kinda

[cobalt ruin]

not sure if im doing it right, im trynna grab teh values with counter b/w 0 and 3 and then update the column in db and then finally do the sendReminder thingy again. Not final implementation obv, wanted to know what im doing wrong before i proceed further.

[drifting oar]

not sure what in looking at, but it looks wrong

im not sure what's unclear. the failure action is to insert the record again (with an increased counter)

nothing else

this is like one line of code

and then u would add if-else to instead log if it was the last failure

makes 3 lines of code

[cobalt ruin]

👀

[cobalt ruin]

i have a few questions, should i pass down the parameters along the method chain? or am i doing it wrong

[cobalt ruin]

is there a way that i can grab the previous record instead of passing all these params

this method will be the caller of reminderFailureAction for context

[drifting oar]

u will have to send it along the path

from where it was read from the db down to that method

[drifting oar]

the general approach now looks better

[cobalt ruin]

ah thank god, took me a while looking at code and figuring out the purpose of most of it

how should i go about this? tempted to make db field static but jjst wanted to know if that would break things or fine

[drifting oar]

as a rule of thumb, never make anything static

in this case, the method shouldnt be static

as simple as that

it was only made static bc it never needed access to fields

if that changes now, it shouldnt be static anymore

[cobalt ruin]

Makes sense, I'll go with that

[cobalt ruin]

i think im stuck in static/non-static hell hole, now it wants me to either make the caller static or instance of database static so that i can call my method

[drifting oar]

if u make this method non-static u also have to make the caller non-static

the caller likely also was made static only bc it didnt need access to fields yet

[cobalt ruin]

that was my initial thought as well, just wanted to check with you just incase the methods were made static with something else in mind and me changing that messes it up further😅

is there a way to simulate failure of reminder command? or would need to write a test for this

[drifting oar]

just literally put a throw new RuntimeException("haha") into the code

or if (Math.random() > 0.5) throw new RuntimeException("haha")

thats the easiest way to test out these things

[cobalt ruin]

ok thanks

[silent ice]

In that case if the error handling is done incorrectly, won't the test pass on a certain attempt, but fail on another?
That does not seem right.

[drifting oar]

yes. this is not meant as unit test. it's for manual testing

[cobalt ruin]

tried throwing exception in both the methods sendReminder and sendReminderRoute, the flow never reaches to consumer for failure. Tried putting a bunch of sysouts in between so im sure it's not hitting those.

console only throws this exception repeatedly

[drifting oar]

u need to throw it in the rest action queue

action.onSuccess(() -> throw...)...queue()

or in a method used in one of the map/flatMap calls

so that it happens within the queue

for example in createReminderEmbed

[cobalt ruin]

onSuccess seems to be working

[cobalt ruin]

am i reading the field of PENDING_REMINDERS table correctly? used map instead of long version and it's not working out

i hope it's not coz of this thingy, i had to typecast it equals only supported integer

[drifting oar]

if u have to cast, things are most certainly wrong

[cobalt ruin]

there's one more thing, regarding default values in REMINDER_FAILURE_COUNTER column, i earlier didnt initialize with a default value so it did null

but if change it with default 0 or INT 0, it throws an error doesnt even begin the application

[drifting oar]

get a db viewer like "Sqlite Browser" and patch the column with it

[cobalt ruin]

it's still throwing the same error

[rapid flax]

dbeaver -_- @cobalt ruin

[cobalt ruin]

you mean i need dbeaver to change the table ?

[rapid flax]

i mean, dbeaver is pretty much the best db viewer imo

at least better than sqlite viewer

[cobalt ruin]

as long as it gets the job done honestly i don't mind

seems like there's some sort of mechanism in place to prevent restructuring of db once it's initialised

[drifting oar]

if u want to have the migration script get executed again, u have to decrement the version

there some special table in the DB holding the current version

wrt the migration script

worst case, u just delete the DB and it gets build fresh - but empty

[cobalt ruin]

that sounds complicated, what if i just deleted the db file and rebuild? would that work

[drifting oar]

worst case, u just delete the DB and it gets build fresh - but empty

[cobalt ruin]

oh lol thought it was related to script

[woven hull]

Closed the thread due to inactivity.

If your question was not resolved yet, feel free to just post a message to reopen it, or create a new thread. But try to improve the quality of your question to make it easier to help you 👍

[cobalt ruin]

..

did a few tweaks and no more typecasting id, anyway is this the correct way to read a certain column from table?

using get just for now, ik it's bad practice will change that later

[drifting oar]

im sure it can be improved. but i dont see any red flags

[cobalt ruin]

ok then i think there's another problem in here, the code never really returns a value

basically im getting a null value

could it be because by the time the action is successful the table is empty

[sweet hull]

Blame him

[cobalt ruin]

blame who?

[sweet hull]

Oh that’s easy man

Oh that’s very easy

/warn @cobalt ruin

[cobalt ruin]

?_?

[viral urchin]

This guy is a troll

[sweet hull]

?

It’s funny, when u cause a massive problem and refuse to accept fault to say it’s everyone else who is the villian @viral urchin

[cobalt ruin]

right keep it to #chit-chat pls

[sweet hull]

Ok we will bring this to there @viral urchin

[cobalt ruin]

it's definitely the block calling for getFailureCounter() is throwing an exception

[cobalt ruin]

Update: i realised what's actually happening here, my getFailureCounter() is failing because when i insert the same record again it's done with a new ID whereas in my method im trynna pull record based on older ID

thoughts on how to go about this? should i forcefully set the ID same as it had before getting removed from DB

or while trynna pull a record from DB which failed, instead of ID should use something else?

[cobalt ruin]

Update: It kinda worked out setting same ID as failed record but had to throw in another one arguement for getting counter, since when i tried to pull the record from DB even with same ID it was already deleted by that time. So i had to grab that counter in sendReminder method and pass it along the chain

as expected it inserts the record back into DB with same ID, and one min delay on next one

now just gotta setup the isLastFailureAttempt method

[cobalt ruin]

i have set up most of the stuff, i ran the tests but it's failing this one particular test

[woven hull]

Closed the thread due to inactivity.

If your question was not resolved yet, feel free to just post a message to reopen it, or create a new thread. But try to improve the quality of your question to make it easier to help you 👍

[cobalt ruin]

..

[drifting oar]

yeah, kinda expected

the feature was fully unit tested, including its behavior on failure

since u made it resilient now, this particular test fails

for obvious reasons

[cobalt ruin]

oh wow

that means im finally done with issue🥹

[drifting oar]

since we kinda consider tests in this project secondary, u can decide whether u want to add tests for it as well or whether u would just delete this unit test for now

[cobalt ruin]

should i do a PR or a draft for review

[drifting oar]

u can just open a PR and we will CR it 🙂

👍

[cobalt ruin]

sonarLint suggesting to not have more than 7 params inside a method, how should i go about this?

[drifting oar]

there's no general way out of this. but u can observe that most of ur arguments are just ur reminder data. so maybe create a record called Reminder or similiar and encapsulate it with that

[shell kindle]

i won't even recommend have more than 5

5 is the worst case

[woven hull]

Closed the thread due to inactivity.

If your question was not resolved yet, feel free to just post a message to reopen it, or create a new thread. But try to improve the quality of your question to make it easier to help you 👍

[cobalt ruin]

Right, I'll try that

[shell kindle]

just send the entire pendingreminder object

[cobalt ruin]

so like store that by reading an entire row and use that later

also i figured out how to setup sonarLint but idk how to do spotlessly

in intellij

[shell kindle]

just run ./gradlew spotlessApply

it should automatically run that on every commit

[shell kindle]

for windows it should be without the ./

[cobalt ruin]

i think ill create a Record for this stuff

since im already creating a record, thinking about refactoring this method as well

[drifting oar]

note that pendingReminder itself is already sth like a record that u could perhaps just continue using

[cobalt ruin]

this si what i did

now i only have to pass this reminder record all along

i tried that command taz mentioned earlier for spotlessly but it showed a few errors on terminal window

for now all my solarLint errors are gone, just need to check with spotlessly

or should i just pass pendingReminder all along ?

[drifting oar]

if thats possible, imo that would be easier lol

[cobalt ruin]

im not sure if that's possible i could give it a try

is this bad tho? what im already trynna do

also kinda confused how to do that spotlessly thing, do i have to declare it as a gradle task? i have checked the wiki, still not sure how to go about it

[drifting oar]

spotless is setup as pre commit hook. that means that its supposed to auto-run when u commit.
I'm not sure how u commit, but u somehow managed to deactivate this feature

it's usually really hard to commit unformatted code lol

that said, u can also always manually run it by just executing gradle spotlessApply or whatever we called it

[cobalt ruin]

i honestly don't know, synced the fork from parent and then pulled from my remote repo and then finally pushed it after commiting ofc

this error popped up

stacktrace looks ugly

[drifting oar]

unsupported major version

sounds like u have wrong java versions setup

ensure its jdk 18

[cobalt ruin]

it's 18 just different vendor

[drifting oar]

well. something in ur chain compiled java 19

63 is 19

so there's now bytecode from 19, which ur jdk 18 doesnt understand

[cobalt ruin]

these are the two settings where i configured 18 specifically

i checked the version for java on terminal, it does say 19. Could it be that gradlew spotlessApply uses that java version instead of the one configured in project structure?

imma try setting up the jdk in gradle.properites file

didn't work out

had to find the task from gradle's task likst

that worked out

does this mean i can commit now, did it work or would do stuff when i commit

[drifting oar]

if spotless runs through fine, that check would likely be green on the PR

[cobalt ruin]

already did PR, not sure if checks need to manually redone

i mean i updated my remote repo, i think it updates the PR as well

it says needs to be done by a maintainer

[drifting oar]

yeah bc u have no contributor permissions yet

[cobalt ruin]

isnt that a life savor lol

[drifting oar]

u get that after ur first successfull PR being merged

[cobalt ruin]

oh shit

too much pressure

[drifting oar]

lmao

i clicked the button, they run now

[cobalt ruin]

omg wow les go

so if these tests pass that means im not gonna break the bot

said it too early

this doesnt make sense, it was supposed to be of type Temporal

[drifting oar]

it doesnt really do anything else than just running spotless and the unit tests (and some sonar checks).
technically u can run all of those commands locally as well

but yeah, if they pass, its good

[cobalt ruin]

i have ruined the second attempt as well

spotlessApply still doesnt make sense, i have ran it twice and it says build successful, let me pull up a screenshot

this is from check build task

ran all of these and none displayed any error, so im confused atm

[cobalt ruin]

@shell kindle here, see for yourself

[shell kindle]

can u just do gradle spotlessApply in terminal

idk what those ide features are

[cobalt ruin]

the problem with terminal is, it runs on java 19 version

[shell kindle]

ik i just like to pretend i don't

[cobalt ruin]

so i had to configure version for gradle specifically via settings

[shell kindle]

see thats y i dont use ide, u don't know how to use ur tools u just know how to use ur ide

[cobalt ruin]

i don't even know anything about gradle, so im not really complaining about IDE atm but isnt that how you run build tasks

[cobalt ruin]

is this need to be done anywhere inside your project? or like has to be specific file

already shared this zabu, he told me that i had java verison 19 then it made sense since terminal refers to version 19 when executing gradle command

[shell kindle]

what u want to do is use a lower version of java

theres so many ways of doing that

changing the java version globally

or just in the project

i believe there is also just for this command

[unkempt coyote]

the only task you have to run (if you don't have pre-commit hook installed for some reason) is spotlessApply

tell us what's the result of that task

also make sure everything compiles, and make sure you actually pushed the latest changes

[cobalt ruin]

not familiar with pre-commit hook so don't think i would configure any of that

it could be that i haven't compiled after the changes maybe that's why build passes

is there a way around this instead of deleting the entire db? this is because i changed the sql file to add column

[cobalt ruin]

interesting, i only used flyway repair. Ill try all of em

quick question, after the spotless check runs does it like "changes code style" on completion or just lets you know that your code failed?

not sure if this worked or not but says successful like the last time i think

i think i might have successfully ran spotlessApply, i do see difference in code format in my local machine vs remote repo like it's all aligned and stuff

[cobalt ruin]

yea it actually works, i tried by putting extra tabs inside code and it changed it back to proper format after running task

[cobalt ruin]

Just did a review request, this should be packup i think most stuff is fixed hopefully

[shell kindle]

lol no

he changed the sql file

[fickle hawk]

the way is to just delete big stupid database and rebuild it

[shell kindle]

i forgot the exact commands but it is one simple google search away

[cobalt ruin]

i had to do it twice that took a lot of time configuring stuff

a lot of suggestions online recommended running flyway repair task

but that didn't work out

it's prolly one of the flyway tasks maybe clean or something

[cobalt ruin]

i might have officially broken a record for longest thread ever in Together-Java

[drifting oar]

but therefore u learned a lot of stuff. and we did as well, by helping u with the problems

glad u figured out most of it now

[cobalt ruin]

im too scared to jump to conclusions now, let's just run the tests whenever you have time😶

appreciate the help btw without your guidance wouldnt have figured out what's going on with the project for sure

[cobalt ruin]

error message when the retry cycle ends

[drifting oar]

is the log message also in there, if u scroll up?

[unkempt coyote]

have you pushed latest changes?

RemindRoutineTest > Skips a pending reminder if sending it out resulted in an error FAILED
    org.togetherjava.tjbot.db.DatabaseException at RemindRoutineTest.java:44
        Caused by: org.jooq.exception.DataAccessException at RemindRoutineTest.java:44
            Caused by: org.sqlite.SQLiteException at RemindRoutineTest.java:44

this test is failing in your PR

[drifting oar]

thats the test that OP was supposed to remove

(or adjust)

[unkempt coyote]

also, feel free to ping me whenever you want me to run the checks again for you

[unkempt coyote]

(makes sense just from reading the log, because test has a nice display name )

[cobalt ruin]

wow the log was hidden in one liner in the crowd

[cobalt ruin]

not yet, thought this was another problem false alarm

quick questions before i commit

our latest commit should have the message it intend to solve?

[unkempt coyote]

not sure what you mean

our contributing has a nice guide on commits and commit messages

[cobalt ruin]

i mean i have done several commits in my repo

so does the commit message stays the same or updates, since it's not merged yet so it technically still solves the same problem

[unkempt coyote]

still don't know what you are asking

can you give an example?

[cobalt ruin]

like i started with initial commit, modified it again after request, modified it again and so on

so in this final commit, teh commit message would still be what it should have been in the Original PR

like fixing the slash reminder thingy

[unkempt coyote]

easiest way is to show your commit history

[cobalt ruin]

ah

let me pull that up

[unkempt coyote]

you don't have to worry about commit history that much, we will squash in the end anyway

[cobalt ruin]

oh

[unkempt coyote]

having a good commit message here and there is helpful tho

[cobalt ruin]

this is just like two out of so many commits

it's just bad history

from now onwards should i update it as i progress or like you said it's gonna get squashed in the end?

[unkempt coyote]

it's getting squashed anyway

it's good for our project because it's beginner friendly

[cobalt ruin]

oh btw this is what i came up with i mean the message, looks alright?

[unkempt coyote]

we don't have to force poor newbies to clean up their git history

yeah

it's important that you have all the changes, and nothing is broken ^^

just try to have few descriptive commit messages here and there, because i will use that as a final message when i squash

[cobalt ruin]

prolly that's why they suggest to have the PR to handle smallest responsibility

[unkempt coyote]

yeh, depends on the workflow

with squashing, that's definitely the goal

since you are destroying a lot of history otherwise

[cobalt ruin]

ill try to commit and write a message then do a push

will check if develop is updated already before that

[unkempt coyote]

it's contrary to the intentions behind git commits

you know, smallest possible and self-contained changes

so you can revert a single bad change, find the commit the introduced the bug, read diffs to understand specific changes, bisect, cherry pick.. etc

if you have whole system in one single commit, kinda removes a lot of version control aspects, the control part

[cobalt ruin]

ye makes sense

what to do with testcases? should i just remove em?

i mean that specific one

[unkempt coyote]

seems like it, or adjust it to make sense with the changes you made

[cobalt ruin]

think i found the flyway thingy to work with changes

nvm it's disabled using a property

this is the one

so there's tiny one unit space that i wanted to remove from my sql script, but flyway checks are killing me

there has to be a civilized way around this, deleting DB or started everything from scratch doesn't sound productive

[drifting oar]

u would have to connect to the DB externally and modify it

for example manually changing its version to the previous version

[cobalt ruin]

for now worked around it, imma remember this one for future reference

about testcases, i don't think i can modify it right away maybe in a later issue.

[unkempt coyote]

we can't merge with tests failing

from the name of the test, it seems like you can safely remove it

[cobalt ruin]

should i comment it out for later to be worked upon or just remove it

[unkempt coyote]

can check the code later

because skipping reminder if errored out is not expected behaviour anymore, we are retrying

[drifting oar]

remove it

[cobalt ruin]

alright ill remove the test and add it in my commit message

[unkempt coyote]

and yeah, zabu confirmed the removal

[drifting oar]

the test was specifically put there to test that failed reminders are dead

but now u repeat them

so the test makes no sense anymore

remove it (and later someone can add a new test which covers the repeat-functionality)

[cobalt ruin]

pushed it

[unkempt coyote]

all checks passed, good job

[cobalt ruin]

wow would you look at that

only took about 400 texts in thread

[drifting oar]

awesome stuff

[cobalt ruin]

it's time to bury this thread in the darkness

Before that thanks to everyone who chipped in

[woven hull]

Closed the thread.

[drifting oar]

in before https://i.imgur.com/1VRGyvQ.png

[unkempt coyote]

he thought it's over

classic mistake

you still need 2 passing reviews before we can merge

[cobalt ruin]

wait what

[drifting oar]

hehe

[cobalt ruin]

common guys im your friendly neighborhood BEE

not sure if i can do that, the write command is creating a new record

so it kinda expects other values

at least doubt it works with newRecord

[drifting oar]

outside of that database.write stuff

if im right, u can get rid of that entirely

[cobalt ruin]

ill try and run it

works alright, not sure about these warnings seeing em for first time

kinda delayed pings as well

[drifting oar]

was it caused by the new code?

[cobalt ruin]

ye saw this first time ever

could it be my internet

ill retry

[drifting oar]

actually, i do see a problem with the databases writeLock not being locked that way

so at least do:

database.write(context -> {
  pendingReminder.set...
  pendingReminder.set...

  pendingReminder.insert();
});

i.e. within the database.write method

[cobalt ruin]

it was temp, everything worked fine this time

[drifting oar]

okay cool. but u still have to put it within the database.write

otherwise we write to the DB without lock

which can cause race conditions

im actually surprised how simple it is to accidentally bypass our DB lock lol

[cobalt ruin]

tested the changes now

[cobalt ruin]

pushed local repo just now