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

[NEW] Live poll: Multi-Question and Timed-Polls #13

Merged
merged 19 commits into from
Sep 14, 2021

Conversation

RonLek
Copy link
Collaborator

@RonLek RonLek commented Aug 10, 2021

  • /poll live <number> subcommand based triggering for live polls added.
  • Live Poll multi-question modal created to allow adding n number of polls.
  • Time to vote (in seconds) provided within modal for user-defined poll end time.
  • Live Poll specific optional properties added to IPoll interface to encourage code-reuse.
  • Next Poll button added within Poll message for triggering next poll to be posted within the room.
  • Updates Apps-Engine version to make use of Scheduler API.
  • Adds "Poll will end " block within live polls message.
  • Registers and calls scheduler (scheduleOnce) to post next poll within room on Time to vote" expiry.

Live Poll

Screencast.from.10-08-21.08.57.40.PM.IST.mp4

Live Poll - Late Retrieval

Screencast.from.16-08-21.01.20.58.AM.IST.mp4

Closes #12

@RonLek
Copy link
Collaborator Author

RonLek commented Aug 10, 2021

@murtaza98 @ramkumarkb please take a look. Thanks.

@RonLek RonLek requested a review from ramkumarkb August 11, 2021 15:38
@murtaza98
Copy link

Couple of suggestions to improve this PR

  1. Improve visibility of this new live subcommand. Eg: adding it to the the param_example
  2. Proper handling of wrong input, eg: Right now, if a user types /poll live or /poll live asdasd, nothing happens. We could handle this in a more user friendly way by eg showing an error message with a sample usage of this command
  3. The additional poll modes ([NEW] Additional poll modes #11) is not available in this PR
  4. The Mixed visibility option ([NEW] Mixed visibility for poll responses #5) is also not available in this PR
  5. Timezone support. Currently in the poll finish message, we're displaying a UTC time, however I think this might get confusing for normal users. I'm aware of the limitation of apps-engine - that it doesn't allow us to know user's timezone - so here's my proposal to get over it. We should add a setting in out app which will allow an admin to choose the timezone which they wish the Poll app to follow and our app should display time in that timezone instead of UTC.

@@ -14,5 +14,10 @@
"description": "A simple app to create polls on Rocket.Chat. Use the slash command: /poll [Question?]",
"implements": [
"IUIKitInteractionHandler"
],
"permissions": [
Copy link

@murtaza98 murtaza98 Aug 22, 2021

Choose a reason for hiding this comment

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

This app uses a lot of other permissions like slashcommand, message.read etc which we need to also declare over here. For a list of all permissions, please refer here and declare ONLY the permissions being used by this app.

PS: Earlier since this app was using apps-engine version < 1.20.0 which is when the permission system was introduced, we didn't have to explicitly declare any such permissions as the apps-engine assigns such apps default permission. However now, since our app is going to be using schedular apis which is on 1.23.0 version of apps-engine - we now need to declare all these permissions explicitly and if this isn't done, the app will start throwing permission errors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the permissions. Please let me know if I missed any. Thanks.

@murtaza98
Copy link

murtaza98 commented Aug 22, 2021

Also there are a lot of tslint errors in this PR so we need to fix them too before we merge this PR. JFYI: You can find the tslint issues by running npm run tslint command

src/PollCommand.ts Outdated Show resolved Hide resolved
src/PollCommand.ts Outdated Show resolved Hide resolved
@RonLek
Copy link
Collaborator Author

RonLek commented Sep 2, 2021

@murtaza98 Mixed Visibilty and Additional Poll modes aren't in the scope of the current PR. To keep things modular I've created a separate issue (#14). I'll raise a PR to that once I have the challenges listed figured out.

app.json Show resolved Hide resolved
@murtaza98
Copy link

murtaza98 commented Sep 12, 2021

Hi @RonLek Apart from the comments, I think also found a bug within /poll live load and /poll live save commands. The issue is when I've created a poll in a particular room and if I run the load command in some other room, it still sends the poll in the original room in which it was created instead of sending it within the room where the user ran that command. Ideally, I think we should allow users to use the same poll across multiple rooms

@RonLek
Copy link
Collaborator Author

RonLek commented Sep 12, 2021

Thanks for the review @murtaza98 . I've resolved all comments and fixed the above issue. Users can now load the same poll across multiple rooms.

@murtaza98
Copy link

That's awesome 💯 Overall this PR LGTM!!

@ramkumarkb ramkumarkb merged commit 79c5e31 into RocketChat:master Sep 14, 2021
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.

[FEATURE] Live Poll: Multi-Question and Timed Polls
3 participants