Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] Make embeds optionnal as those can be hidden clientside #21

Merged
merged 1 commit into from
Mar 27, 2020

Conversation

arbaes
Copy link
Contributor

@arbaes arbaes commented Mar 26, 2020

If the users have hidden the embed messages in their settings,they only see the reactions attached to an empty message.

This commit aims to make the embed messages 'optionnal' and rely on a normal message. Here's the changes on the commands:

{prefix}new - message formatting (step == 3) :

  • some text: Set only a message
  • some text // some embed title: Set a message with an embed that only has a title
  • some text // some embed title // some embed content: Set a message with an embed that has a title and some contents

{prefix}edit:

  • {prefix}edit #channelname // MESSAGE_NUMBER // New Message: Change the message's body
  • {prefix}edit #channelname // MESSAGE_NUMBER // New Message // New Embed Title (Optionnal):
    • If the reaction-role message already had an embed, it keeps its existing description, changes its title and the message's body.
    • If the reaction-role message didn't had an embed, it sets one with only a title and the message's body
  • {prefix}edit #channelname // MESSAGE_NUMBER // New Message // New Embed Title (Optionnal) // New Embed Description (Optionnal)t:
    • If the reaction-role message already had an embed, it changes its title, its description and the message's body
    • If the reaction-role message didn't had an embed, it sets one with the new title, the new description and the new message body.

{prefix}rm-embed:

  • {prefix}edit #channelname // MESSAGE_NUMBER: Remove the embed set on the selected reaction-role message

@arbaes
Copy link
Contributor Author

arbaes commented Mar 26, 2020

@eibex I'm sorry 😞. This PR ended up way bigger than I expected, dont hesitate if you have any remarks

@eibex
Copy link
Owner

eibex commented Mar 26, 2020

Thanks for the PR! It makes sense and looks good.

I will test the PR once I have time this evening or tomorrow and also see if it is easily adaptable for #20, but I doubt any major issues would arise.

bot.py Outdated
f"`{prefix}edit #channelname // 1 // New Title // New Description` "
"to edit the reaction-role message."
"There is only one reaction-role message in this channel. **Type**:"
f"\n`{prefix}edit #channelname // 1 // New Message // New Embed Title (Optionnal) // New Embed Description (Optionnal)`"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: it should be optional

bot.py Outdated

await ctx.send(
f"There are **{len(r_ids)}** reaction-role messages in this channel. **Type**:"
f"\n`{prefix}edit #channelname // MESSAGE_NUMBER`"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be {prefix}rm-embed not edit

@eibex
Copy link
Owner

eibex commented Mar 26, 2020

Found some time to test it now.

Other than the very minor issues I already sent, I noticed that if you try to rm-embed a message that was created before this PR (i.e. does not have a message, only an embed) an HTTPException: 400 Bad Request (error code: 50006): Cannot send an empty message is raised. I think it should be caught and the bot should reply to the user to first edit the reaction-role message to add some text on top of the existing embed.

Other than that it works great, thanks a lot!

@arbaes
Copy link
Contributor Author

arbaes commented Mar 27, 2020

Or we could also delete the message in that case ?
But I think your idea is better because it's probably better to keep the "less destructive" approach.

@eibex
Copy link
Owner

eibex commented Mar 27, 2020

Rewriting the response since I think I misunderstood your comment.

I wouldn't straight up delete the message without warning since from what I can see it's meant to maintain it but just remove the embed.

After implementing #20 I would work on a new remove command and fix #3. So I would just go for the user warning route for this.

@eibex
Copy link
Owner

eibex commented Mar 27, 2020

Thanks for the quick changes!

@arbaes
Copy link
Contributor Author

arbaes commented Mar 27, 2020

@eibex I did the changes. I also slightly modified the new command. to allow creating role-reaction message with an empty message ( but with an embed ). And now I'm thinking I maybe should have done that for the editcommand too. What do you think ?

@eibex
Copy link
Owner

eibex commented Mar 27, 2020

That would be a good idea for completeness. Thanks! But I would wait to merge #20 first to avoid continuous merge conflicts (sorry!).

eibex pushed a commit that referenced this pull request Aug 23, 2020
eibex pushed a commit that referenced this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants