Fix for Issue #191

[infinitepower18]

Here is the fix for Issue #191. The problem was caused due to Discord's character limit, if the user has too many roles the thread creation process won't work properly. This PR sets a limit on how many roles the bot can display before it says "Too many roles to show" therefore not exceeding Discord's character limit. I have set the value to 21, meaning it can display up to 20 roles before it says too many roles.

Initially I tried setting the value to 41, but then the embed message of where the user info and roles are shown wasn't sending at all, so I reduced the value to 21 and now it's working fine.

[fourjr]

Don't set a hard limit. what if people has rolenames longer than that?

[infinitepower18]

It is similar to what Dyno does when using whois on a user with a lot of roles.

[fourjr]

i dont think that is relevant to modmail?

[infinitepower18]

It will at least solve the issue, so best to deploy this now, if needed changes can be made later

[x-frst]

For your question @fourjr if any user has more roles, then just change value in if condition as of now it is set to 21 roles including the @everyone role you can change it to a specific limit but as we tried it didn't work for more roles and same problem was happening so this is a basically a quick fix! However devs will surely work on this thing in future commits...

[fourjr]

I suggest checking the size of the description and ensure it does not cross the limit.

[kyb3r]

Instead of using a role number limit it would be more robust to use a character limit.

[infinitepower18]

Will see what can be done and report back.

[infinitepower18]

After doing some extended testing and modifications, here is the final version of the thread.py file.

Some things to point out:

1. In non-separate server mode, the amount of characters in the Discord role name does not matter as it uses IDs to take up the character limit. I tried setting the value to 46, but it was exceeding the 1024 character limit. 41 is fine though (meaning 40 roles will be displayed excluding everyone role).

2. In separate server mode, this is where character count is important unlike the non separate mode where it uses mentions so only IDs take up the character count. The bot can handle up to 1024 characters in a single embed. But at the same time, need to make space for other characters in the embed so I decided to allow up to 800 characters (including roles, commas and spaces in total) that can be displayed, so there is enough room for the other characters in the embed, including the full username which can go up to 32 characters.

3. A role count is also added.

This is the best that could be done, hope this is sufficient.

Below is a screenshot on what the thread creation embed looks like when a user has more than 40 roles, in non-separate server mode. Role count is also there.

[fourjr]

Wouldn't a simple value[:1000] + ('' if len(value) <= 1000 else '...') work?

[infinitepower18]

You were right. I just checked the Discord developer docs and the field.value, which is where all the roles are displayed can't exceed more than 1024 characters. I edited the file and set the character limit to 1024 for both separate and non-separare server modes. Now I'm fully confident it is ready to merge and deploy the update.

[fourjr]

count += 1

[fourjr]

count -= 1

[fourjr]

use snake_case

[fourjr]

i think it's best to show what we can & put ellipsis after

[kyb3r]

Wouldn't a simple value[:1000] + ('' if len(value) <= 1000 else '...') work?

This would mess up role mentions, I think it'd be better to iteratively concatenate role names/mentions until the length of the string is just under 1024.

e.g.

roles = [] for role in user.roles: fmt = role.name if sep_server else role.mention if sum(len(r) for r in roles) + len(fmt) > 1000: roles.append('...') break roles.append(fmt) seperator = ', ' if sep_server else ' ' roles = seperator.join(roles)

[fourjr]

What about commands.Paginator? It implements that iirc

[x-frst]

@fourjr My opinion is you create a new pull request by your own implementation and show that to us would be great for you & everyone :D

[infinitepower18]

What else can be done differently? My latest commit I have tested it in both separate and non-separate mode and I believe it is fine the way it is. The problem was the field.value where the role lists are displayed can only go up to 1024 characters according to Discord developer docs. So I made it only display up to 1024 characters if it goes more than that it will say can't show roles but it will at least not crash and cause problems for some servers.

Please test it by creating a thread for a user that has a lot of roles and see if anything needs to be changed but I'm pretty sure there's nothing else that need to be done and ready for deployment.

[infinitepower18]

And before starting to edit the thread.py file I checked what the original problem was by reproducing the bug and seeing what comes up in the logs:

2019-03-19T08:39:16.782389+00:00 app[worker.1]: future: <Task finished coro=<Thread.setup() done, defined at /app/core/thread.py:79> exception=HTTPException('BAD REQUEST (status code: 400): Invalid Form Body\nIn embed.fields.0.value: Must be 1024 or fewer in length.')> 2019-03-19T08:39:16.782418+00:00 app[worker.1]: In embed.fields.0.value: Must be 1024 or fewer in length. 

You can see it was the field.value exceeding 1024 characters and due to this the thread creation embed won't show at all. With my latest commit this problem doesn't happen anymore.

[fourjr]

We don't develop modmail just for "it to work" and getting the fastest solution. It is also for style and the cleanest possible code. If you feel that you want to stick to your messy code, feel free to do so in your own fork.

[infinitepower18]

Here's a cleaner code. If you said you wanted clean code we could have done it before. Anyway here it is. Let us know if there's anything else.

[kyb3r]

A bit late but I implemented this in 2db8b85