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

Button for others to opt-in to be notified for reminders #2973

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Commits on Mar 24, 2024

  1. Reminders: Add a button for others to opt-in to a ping

    This is the initial implementation - it's currently _far_ from perfect
    and is very susceptible to errors.
    
    Notable features beyond the basic requirements:
    
    - Fails safely when max mentions reached (we're limited by the
      2000-character limit when sending the reminder with the pings), and
      disables the button.
    
    - Adds an additional embed to the initial confirmation message to show
      who clicked on the notify button.
    
    - Edits the additional embed and disables the button when the reminder
      is sent.
    
    In many ways, this implementation is quite bad:
    
    - Uses an async callback to delegate the task of PATCH-ing the API to
      edit mentions to the `new_reminders` method.
    
    - Edits to the opt-in list embed relies on the fact that the reminder is
      not edited (using !remind edit) before someone clicks on the button. A
      trivial way to fix this would be to add another field to the site
      schema to store the `notification_view` in some way.
    
    - The button is neither disabled nor any edits to the embed made when
      the reminder is deleted before someone clicks on the button.
    
    - String splitting is used which relies on the exact format of the embed
      message when editing the embed to disable the button. We have to
      reminder to update this piece of code when adjusting its format in the
      future.
    
    The UX can also be improved. Currently, I can't think of a way to
    concisely phrase the button embed message so that it is clear that the
    button is for people _other than_ the reminder author.
    
    Notes:
    
    - Max reminder mentions:
    
      - Mentions are pinged directly in a discord message when the reminder
        is sent. This means we're limited by the 2000-char limit. If we take
        each User ID snowflake to be 18-characters, and considering each
        mention to be formated as "<@id> " (with extra space), it results in
        about 90 mentions max. I've set the constant to 80 just in case.
    
      - This is not an issue when the mentions are added in through other
        means than the button we're adding in this commit, because the user
        has to use @-mentions when sending the `!remind edit` command, which
        is already under the discord's character limit.
    
    - Log messages are added when something unexpected occurs within the
      code. Hopefully this is unlikely to happen after the implementation
      issues listed above are solved.
    
    - The opt-in list in the second embed is separate from mentions added in
      the original reminder creation, or any further edits, because mentions
      are added by the to-be-mentioned-user, rather than by the reminder
      author in this way. (Even though they are stored the same way.)
    hedyhli committed Mar 24, 2024
    Configuration menu
    Copy the full SHA
    e7780bd View commit details
    Browse the repository at this point in the history

Commits on Mar 25, 2024

  1. Reminders: More robust implementation of mention opt-in button

    This solves most, if not all issues from the previous commit.
    
    - A timeout of 5 minutes is enforced - this means the button can no
      longer be used either when the reminder arrives or 5 minutes passes
      since creation, whichever comes first.
    - Reminder edits in between creation and button clicks will be handled
      responsibly
      - This includes both edits of duration, mentions, and deleting
        reminders altogether.
    - UX is improved. This list of to-be-mentioned users is sent up-front
      with the author included. Instructions to click the button comes right
      after the list.
    - No updates to the API or site schema required, as the button message
      will disable itself when it encounters any sort of errors.
    - Implementation is also somewhat simplified.
    
    There are probably more improvements, maybe one caveat, but it's like
    almost midnight and I want to sleep :/ I sure hope the list above covers
    most of it.
    
    Further testing will be done. Now `.remind 10s test` is ingrained in my
    muscle memory...
    hedyhli committed Mar 25, 2024
    Configuration menu
    Copy the full SHA
    cdaa59f View commit details
    Browse the repository at this point in the history

Commits on Mar 26, 2024

  1. Reminders: Refactor all opt-in button related logic into View class

    - More resilient handling of API errors.
    - Don't rely on string manipulation to disable the button.
    hedyhli committed Mar 26, 2024
    Configuration menu
    Copy the full SHA
    e753586 View commit details
    Browse the repository at this point in the history

Commits on Mar 27, 2024

  1. Reminders: Simplify helper function to get button embed

    This removes the need of any form of error handling from `embeds` list
    indexing, and makes the code more concise and readable.
    
    The drawback is that a whole new embed object have to be allocated each
    time, rather than editing the description like before.
    hedyhli committed Mar 27, 2024
    Configuration menu
    Copy the full SHA
    8efc22d View commit details
    Browse the repository at this point in the history