-
Notifications
You must be signed in to change notification settings - Fork 102
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 preferences slash command #127
Conversation
Hello @darkLord19, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
==========================================
+ Coverage 16.60% 16.80% +0.20%
==========================================
Files 15 15
Lines 1331 1416 +85
==========================================
+ Hits 221 238 +17
- Misses 1069 1136 +67
- Partials 41 42 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I have some change requests that I'd like to see addressed before merging!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes requested
Co-authored-by: Jason Frerich <jason.frerich@mattermost.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darkLord19, thanks for the quick turn around! There is a listing error that needs to be fixed. You can run make check-style
to confirm see any errors and confirm the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for jumping on this issue 👍
A few comments below
server/command_hooks.go
Outdated
return &model.CommandResponse{} | ||
} | ||
for _, p := range prefs { | ||
if p.Name == name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at the content of prefs
it doesn't seem like we can easily match on Name
:
{
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "tutorial_step", Name: "iqbqga6mdjfa3prh59er8fh57c", Value: "999"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_approximate_view_time", Name: "", Value: "1605693815487"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "m93njr5obbyqirzf6raixu63hw", Value: "1604692117885"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "edi3ewpbcpru5gftmpxrhna31o", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_approximate_view_time", Name: "xnu1rgdpiffwjfb15sgdnhuaho", Value: "1604314238495"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "theme", Name: "", Value: "{\"awayIndicator\":\"#ffbc42\",\"buttonBg\":\"#166de0\",\"buttonColor\":\"#ffffff\",\"centerChannelBg\":\"#ffffff\",\"centerChannelColor\":\"#3d3c40\",\"codeTheme\":\"github\",\"dndIndicator\":\"#f74343\",\"errorTextColor\":\"#fd5960\",\"image\":\"/static/files/460473bcf310d651a1d68e35e484df97.png\",\"linkColor\":\"#2389d7\",\"mentionBg\":\"#ffffff\",\"mentionColor\":\"#145dbf\",\"mentionHighlightBg\":\"#ffe577\",\"mentionHighlightLink\":\"#166de0\",\"newMessageSeparator\":\"#ff8800\",\"onlineIndicator\":\"#06d6a0\",\"sidebarBg\":\"#145dbf\",\"sidebarHeaderBg\":\"#1153ab\",\"sidebarHeaderTextColor\":\"#ffffff\",\"sidebarText\":\"#ffffff\",\"sidebarTextActiveBorder\":\"#579eff\",\"sidebarTextActiveColor\":\"#ffffff\",\"sidebarTextHoverBg\":\"#4578bf\",\"sidebarUnreadText\":\"#ffffff\",\"type\":\"Mattermost\"}"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "h7qmwf3d7tb9mqtwzpa7njgapr", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "uxhczcwh3pfytj9dh5fu6bhp1h", Value: "1604575010094"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "recommended_next_steps", Name: "skip", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "iqbqga6mdjfa3prh59er8fh57c", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "umwssh8uy7fcdxoghsgwcst9we", Value: "1604496816318"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_approximate_view_time", Name: "son1npofqidwxjkodkrjbb44ua", Value: "1604400840350"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "g64gf5qtk7dp7cnxcy1p8k9g4o", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "kjkbt4z8kfbuuyt7h1qmy7ebuy", Value: "1604056605335"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "1i9xpftodjbn3mspx3zgnjj6gc", Value: "1604918656936"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "8se64adhfb8kdkbc9up6rqi5cc", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "rf76s354mbg73mhsjpcnikzyew", Value: "1604563673011"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "i35936p3cty7jyys88jidycmfh", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "channel_open_time", Name: "pmpc15r9f3yh5pk6ad4t9bwpsw", Value: "1604496817135"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "recommended_next_steps", Name: "hide", Value: "true"},
model.Preference{UserId: "iqbqga6mdjfa3prh59er8fh57c", Category: "direct_channel_show", Name: "5k91qhwi9fbixq77cknfm3ywia", Value: "true"}
}
Could you please investigate if there is a way to map the preference to a human readable text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think category could be the actual name for preference because Name is just a UUID. Need to see how it is set across the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hmhealey Maybe you can help here. Do you know what kann be used as a human readable text to set/get specific preferences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The category/name are both arbitrary strings, so some are human readable while others aren't. For ones with the category direct_channel_show
, the name is the user ID if the other user in the DM, but for ones with the category sidebar_settings
, the name will be something readable like show_unread_section
.
Preferences is basically a key/value store where the key is made up of the combination of category and name, so you'd want both the category and name to address a preference, but the category also lets us group related preferences in the case where you want something like all DMs with visible channels in the sidebar.
server/command_hooks.go
Outdated
return &model.CommandResponse{} | ||
} | ||
for _, p := range prefs { | ||
if p.Name == name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hmhealey Maybe you can help here. Do you know what kann be used as a human readable text to set/get specific preferences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM regarding my PR change requests. Approving my end, but will need to address the remaining requests from Ben and Harrison
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@hanzei Perhaps what we could do is change the command to be like |
@hmhealey In that case, how would a user remember the |
Some preferences still have human-readable names like the |
Good point. @darkLord19 What do you think? |
@hanzei makes sense. I will have a go at implementing it once I have some free time on my hands. |
Hi @darkLord19 - I'm helping to review stale PRs and am wondering whether you're able to prioritize this? Is there anything our devs can help with to push this over the finish line? |
Closing as stale. @darkLord19 Thank you very much for working on the issue. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #127 +/- ##
==========================================
+ Coverage 16.60% 16.80% +0.20%
==========================================
Files 15 15
Lines 1331 1416 +85
==========================================
+ Hits 221 238 +17
- Misses 1069 1136 +67
- Partials 41 42 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Fixes mattermost/mattermost#14634