first gambling commit by ludvigsandberg · Pull Request #86 · Better-Cpp/Better-C-Bot

ludvigsandberg · 2023-12-18T23:50:53Z

No description provided.

Tsche · 2023-12-22T13:35:32Z

src/cogs/gamble.py

+            await ctx.send('You must enter a positive number greater than 0.', ephemeral=True)
+            return
+        if amount > user_database_get('money', ctx.author.id):
+            await ctx.send('You don\'t have enough money.', ephemeral=True)


Rather than escaping you can use "You don't have enough money.". We've done this pretty arbitrarily so far but ig for longer strings double quotes should be preferred

Tsche · 2023-12-22T13:36:58Z

src/cogs/gamble.py

+        user_database_check_if_user_exists_otherwise_add(user.id)
+
+        embed = discord.Embed(title=f'{user.display_name}\'s wallet', color=discord.Color.blurple())
+        embed.set_thumbnail(url='https://cdn.iconscout.com/icon/free/png-512/free-wallet-588-456730.png')


What's the license for that image? If you want we can add it as emoji or sticker so we don't have our bot link to something off-platform that's also entirely out of our control

Tsche · 2023-12-22T13:37:41Z

requirements.txt

 discord.py==2.1.0
 aiohttp==3.8.5
 python-Levenshtein==0.12.2
+datetime==5.4


Why do we need a package for that? Isn't python standard library datetime utils sufficient?

Tsche · 2023-12-22T13:38:26Z

src/cogs/gamble.py

+from src.util.user_database import *
+
+
+daily_amount = 10


please put configuration options in config.py

Tsche · 2023-12-22T13:38:54Z

src/cogs/gamble.py

+        """
+        Place a bet (50/50). Enter the amount to bet.
+        """
+        user_database_check_if_user_exists_otherwise_add(ctx.author.id)


that's one long af identifier

user_database_upsert is better

Tsche · 2023-12-22T13:45:31Z

src/cogs/gamble.py

+    @commands.hybrid_command(with_app_command=True)
+    async def give(self, ctx: Context, user: Member, amount: int):
+        """
+        Transfer money to another member. First argument is user ID. Ssecond argument is amount.


Ssecond typo

Tsche · 2023-12-22T13:46:47Z

src/cogs/gamble.py

+        # Poor man's clamp.
+        entries = max(1, min(entries, 50))
+
+        query = user_database.execute(f'SELECT id, money FROM users ORDER BY money DESC LIMIT ?', [entries])


you have extracted all database queries except for this one, why?

Tsche · 2023-12-22T13:47:24Z

src/util/user_database.py

@@ -0,0 +1,31 @@
+import sqlite3
+
+user_database_connection = sqlite3.connect('users.db')


database path should be configurable (in config.py)

This file is non-persistent

Tsche · 2023-12-22T13:51:32Z

src/util/user_database.py

namespacing by prefixing everything with user_database is yucky. Please wrap it in a class instead (or drop the prefix entirely and import the module instead of its contents to force usage of its name).

I don't like how it does a lot of stuff when importing the module (in Python modules are essentially singletons). That should not be required and will make testing a lot harder if we ever decide to write tests for any of this. If you want to ensure there's exactly one instance of this, you should probably open up the database connection during bot initialization instead.

Tsche · 2023-12-22T13:52:03Z

src/util/user_database.py

+    count = query.fetchone()[0]
+
+    # If no user ID was found in database, add user.
+    if count == 0:


this should be if not count

Still need to fix /daily and turn the wallet icon into a server sticker.

Still need to update the sticker id in config.py after Che uploads the sticker.

YuhanunCitgez · 2024-02-12T21:35:39Z

@Tsche we still wanna move ahead?

Tsche · 2024-02-13T04:07:26Z

@Tsche we still wanna move ahead?

I'm currently super busy but in general yeah sure why not. Not sure what the current state of this PR is, I'll try to look into it some time this week.

makian123 · 2024-03-08T13:41:45Z

Is the input properly sanitized, since its interacting with database (amount to gamble cant be a string and where it accepts string it cant be like ;drop table users)

ludvigsandberg · 2024-03-09T21:06:37Z

@makian123 SQL injections won't be a problem as long as we use parameterized queries with question marks instead of raw string operations

makian123 · 2024-03-09T21:29:53Z

Alright, that is pretty cool

makian123 · 2024-03-14T14:51:01Z

you need to modify the docker run args so it takes in a volume, that way sqlite database will be persistent

Pesistent Docker Volume

makian123 · 2024-05-19T09:24:49Z

I have one big issue with this, your database is non scalable, the class you've written in utils will only work for the gambling stuff, can you generalize the DB class so it uses tables and you could set field in a custom way? That way you can just write a wrapper for your gambling.
Requesting this because i have some moderation feature plans in future and it'd make it easier to have one sqlite database with different tables

ludvigsandberg · 2024-05-19T11:12:45Z

@makian123 moved gambling specific functions from db.py into gamble.py.

first gambling commit

2bbfd80

Tsche requested changes Dec 22, 2023

View reviewed changes

ludvigsandberg added 3 commits December 23, 2023 15:23

gambling related improvements

875e3ca

Still need to fix /daily and turn the wallet icon into a server sticker.

Fixed /daily and sticker in embed

43aa5de

Still need to update the sticker id in config.py after Che uploads the sticker.

Fixed all of Che's requests.

8c39d3a

ludvigsandberg and others added 4 commits May 19, 2024 09:43

Moved database.

4c92d8c

persistent volume

473fa23

persistent volume

d0357a1

Merge pull request #2 from bizxeon/master

750a121

Pesistent Docker Volume

Moved gamble specific functions from db.py

96e9d40

		@@ -0,0 +1,31 @@
		import sqlite3

		user_database_connection = sqlite3.connect('users.db')

Conversation

ludvigsandberg commented Dec 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuhanunCitgez Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuhanunCitgez commented Feb 12, 2024

Uh oh!

Tsche commented Feb 13, 2024

Uh oh!

makian123 commented Mar 8, 2024

Uh oh!

ludvigsandberg commented Mar 9, 2024

Uh oh!

makian123 commented Mar 9, 2024

Uh oh!

makian123 commented Mar 14, 2024

Uh oh!

makian123 commented May 19, 2024

Uh oh!

ludvigsandberg commented May 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

YuhanunCitgez Mar 9, 2024 •

edited

Loading