"Archive Option" Feature

abrahammurciano / paul-bot

Hi! I'm Paul; a teeny tiny bot who's good at one thing. Making polls. And when I say good, I mean really really good. Like the bestest best bot ever at making polls. I can make open polls, I can make closed polls, I can make dynamically editable polls, and I can make them any size. And all that with a simple, good-looking, and easy to use interface! So come on and try me out in your server!

GNU General Public License v3.0

17 stars 7 forks source link

"Archive Option" Feature #32

Open mattyg opened 2 years ago

mattyg commented 2 years ago

Overview

Feature to allow a discord user with editor permission to archive a poll option, copying the UX of adding an option.
created an .env.example and basic developer documentation
move sql schema definition into migrations folder, to make their order explicit

Deployment

Run migrations/1_archive_options.sql on database before deploying code changes

WIP This currently introduces a bug where an VoteButton can display the wrong index under certain condition -- when there are 3+ VoteButtons and the 1st or 2nd is archived. I don't have the bandwidth to resolve it right now. I removed the index number from the VoteButton text as a workaround that's enough for my use case. Edit: fixed

abrahammurciano commented 2 years ago

Thanks for this! I'll review it sometime in the next few days hopefully

abrahammurciano commented 2 years ago

@mattyg What's the intention of this feature? Is it to delete an option so that it is no longer visible and no record of its existence is visible to the user? Or is it simply to disable voting on said option?

If it's the former, then the option and all its votes can be deleted from the database (database should delete votes automatically, I think cascade delete is enabled), and this feature can be called "remove option" rather than "archive". (Regarding option indices, I think the option index can be removed from the database. IIRC I added them to fix a bug that could have been, and subsequently was, fixed another way.

If it's the latter, then I would say that the options should all stay in the same list, and they should all be displayed, but some indication that it is archived (like the word "archived" in parentheses, and the option label ~~crossed out~~, as well as the vote button being grey) should be displayed instead of not displaying it at all.

I'm okay with either one being implemented (preferably not both because they achieve basically the same thing). So which one is it?

mattyg commented 2 years ago

@mattyg What's the intention of this feature? Is it to delete an option so that it is no longer visible and no record of its existence is visible to the user? Or is it simply to disable voting on said option?

Yes, good point. My intention was to display the list of archived options, underneath the list of "active" options.

I updated this PR to do so, and also fixed the bug I noted in the original comment. (heads up that I modified the migration 1_archive_options.pqsl, so it will have to be re-run on top of the original db state)

I guess that while "functionally" we are archiving the option (i.e. it is not deleted in the db), from the user's perspective it might as well be removing the option, since they cannot un-archive it. It may make sense for the discord UX to use the language "Removed" rather than "Archived". Happy to just follow your preference on that.

abrahammurciano commented 2 years ago

Very well. In that case I'd rather delete the option entirely from the database (and cascade delete the votes) as it would just be taking up unnecessary space. There's no need to display the deleted options. It would also make it a lot simpler to deal with option indices.

I would like to remove the index column from the option table (and class) entirely, and instead rely on the order that the options are returned by the query. If you'd like to do that, it will make it easier for you to delete options.

I'll finish reviewing your actual code soon, but in the meantime you can make it do a full proper delete instead of archiving.

abrahammurciano commented 2 years ago

Also I'm not too keen on allowing the same people who can add options to delete options. Adding an option is less harmful than deleting one. If you have a poll and want to allow anyone to suggest more options, you don't necessarily want to allow anyone to remove other people's options.

Possible solutions might include:

have a separate option called allowed_deleters or something (most flexible solution, but I'd rather keep the interface as simple as possible)
only allow poll author and option author to delete an option (just like only poll author is allowed to close the poll)
piggyback on discord's delete messages permission (might be worth doing the same for closing polls) to determine who can delete options
combination of previous two solutions (my favourite solution)
allow any editor to delete options, but only if (new) argument allow_deleting_options is set to true (default to false)

mattyg commented 2 years ago

To be clear, for my use case, I want to display a list of the archived options. If that doesn't fit with your vision for the project please let me know.

This archive feature doesn't affect use of the index column. It does rely on the order that the options are returned by the query.

My preference for revising permissions is option 2, for simplicity. Honestly I don't the bandwidth to do any more significant refactoring. Please confirm if that's a reasonable compromise for you and I'll implement it.

abrahammurciano commented 2 years ago

Fair enough. That is acceptable. In that case you may keep the wording as "archive" instead of "remove".

Thanks again for your contribution! Really appreciate it

mattyg commented 2 years ago

Modified to only allow poll author to archive options.