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

Add RHS informations card to show users votes #431

Merged
merged 9 commits into from Jul 16, 2022

Conversation

jbattistispiria
Copy link
Contributor

@jbattistispiria jbattistispiria commented May 31, 2022

image

  • Add a new option when creating a poll «Show Voters».
  • Add error when anonymous option is use with show voters option.
  • Add unit tests

Change made after comments:

  • Add extra information when using the --progress setting

Show extra information on who voted for what during poll in the RHS feed.
The extra information is send in markdown by using a "card" props from the Mattermost API.
Mattermost post add the info icon to the post and create the RHS.

Fix #96

@kaakaa kaakaa self-requested a review June 15, 2022 13:40
@kaakaa kaakaa added Type/Enhancement New feature or request 3. To Review Docs/Needed Requires documentation Tech/Server Plugin Server labels Jun 15, 2022
@kaakaa kaakaa added this to the v1.6.0 milestone Jun 15, 2022
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #431 (0bbbc56) into master (f7a2499) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
+ Coverage   93.63%   93.78%   +0.14%     
==========================================
  Files          14       14              
  Lines        1289     1320      +31     
==========================================
+ Hits         1207     1238      +31     
  Misses         55       55              
  Partials       27       27              
Impacted Files Coverage Δ
server/plugin/api.go 91.25% <100.00%> (+0.15%) ⬆️
server/plugin/command.go 98.90% <100.00%> (+0.01%) ⬆️
server/poll/transform.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7a2499...0bbbc56. Read the comment docs.

Copy link
Contributor

@kaakaa kaakaa left a comment

Choose a reason for hiding this comment

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

Great 👍 This feature works fine for me, and your changes almost looks great! I left a few review comments and would appreciate your response.

And Let me make one suggestion. How about including the functionality of --show-voters in the existing --progress setting? Separating those two poll settings would make --progress just display the number of votes on buttons, which does not seem useful. And I don't want to increase the number of poll setting as much as possible. How does this sounds for your use case?

Copy link
Contributor

@kaakaa kaakaa left a comment

Choose a reason for hiding this comment

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

I forgot to send comments.

server/poll/transform.go Outdated Show resolved Hide resolved
server/plugin/api.go Outdated Show resolved Hide resolved
@jbattistispiria
Copy link
Contributor Author

About including the functionality of --show-voters in the existing --progress setting, i thought the same thing as you at first and i did the code that way, but i wasn't sure if people using --progress would want to have the extra information sidebar.
So i keep the two settings separated.
I'm open to make the change, let me know if that's go.

@kaakaa
Copy link
Contributor

kaakaa commented Jun 17, 2022

Thanks for sharing your thought, and I understand it. But I still be inclined to include this functionality in --progress. My thought is the following.

  • The card is not visible unless a user click on it, so it is not obtrusive
  • A poll with --progress makes the voting user name public as a result, so I don't think it will be a problem if the voting user name is made public during a poll is open
  • If a poll is created with --progress and --anonymous, with existing implementation, the voting user name will be hidden and anonymity is protected
    • In this case, a card doesn't have any useful information, but it would not be bad

If I seem to have missed something, let me know it.

@kaakaa kaakaa mentioned this pull request Jun 17, 2022
@jbattistispiria
Copy link
Contributor Author

Great. I'll completely agree with you and i will move show-voters with progress.

This converter add more informative on mouse over a name
Use bundle.LocalizeWithConfig instead of MustLocalize,
because MustLocalize causes panic if translation is missing.
@jbattistispiria
Copy link
Contributor Author

jbattistispiria commented Jun 20, 2022

I made the change to include the functionality of --show-voters in the existing --progress setting.

Copy link
Contributor

@kaakaa kaakaa left a comment

Choose a reason for hiding this comment

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

Thanks 👍 There are 2 additional points that I would like to fix, so please approve them if they looks good.

server/plugin/command.go Outdated Show resolved Hide resolved
server/poll/transform.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kaakaa kaakaa left a comment

Choose a reason for hiding this comment

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

Perfect 👍

@kaakaa kaakaa added 4. To Release Do Not Merge Should not be merged until this label is removed and removed 3. To Review labels Jun 22, 2022
@kaakaa
Copy link
Contributor

kaakaa commented Jun 22, 2022

Since Matterpoll is on a way to release v1.5.0, I'd like to wait to merge this PR until the release process moves forward. If the release process seems to take a long time, I'll cut a new branch for release-1.5 and merge this. I'm careful not to generate additional work for this PR.

@kaakaa kaakaa mentioned this pull request Jul 9, 2022
@kaakaa
Copy link
Contributor

kaakaa commented Jul 16, 2022

Since there is no activity for a month in mattermost/mattermost-marketplace#298, move forward this PR. Thanks again for your contribution @jbattistispiria!

@kaakaa kaakaa merged commit a5fdb7d into matterpoll:master Jul 16, 2022
@kaakaa kaakaa removed the Do Not Merge Should not be merged until this label is removed label Jul 16, 2022
@hanzei
Copy link
Member

hanzei commented Sep 18, 2022

@jbattistispiria This is great, thanks for the contribution. 🎉

Quick q: Does this feature work on mobile?

@kaakaa
Copy link
Contributor

kaakaa commented Sep 21, 2022

No. AFAIK, card props is not supported on Mobile, so RHS information cannot be seen on mobile.

@q-wertz q-wertz mentioned this pull request Mar 9, 2023
kaakaa added a commit that referenced this pull request Apr 14, 2023
kaakaa added a commit that referenced this pull request May 2, 2023
* docs: update how to install goi18n

* Update readme for #431

* bump up version

* clean up readme
@physiker05
Copy link

Hi all! Although a bit outdated... I'd like to know how I get to see this card that shows me the users participating in an open poll? I started a poll using the option --progress, but I don't get to see the stats. Your help is very much appreciated!

@kaakaa
Copy link
Contributor

kaakaa commented Apr 23, 2024

@physiker05 I'm sorry that I didn't catch your comment.
First let me check, what version of matterpoll are you using? If you are using Matterpoll v1.6.0 or later, you should be able to see the icon to open the card by adding the --progress option.

open-card

@kaakaa kaakaa added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Apr 23, 2024
@physiker05
Copy link

@kaakaa Thanks for getting back to me! Well, you probably identified the main issue... I'm running on v1.4.0 😁.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. To Release Docs/Done Required documentation has been written Tech/Server Plugin Server Type/Enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Show how voted during poll via Poll Setting
4 participants