Closed ayusht2810 closed 1 day ago
Attention: Patch coverage is 41.22807%
with 134 lines
in your changes missing coverage. Please review.
Project coverage is 16.56%. Comparing base (
d73eafa
) to head (2549a1e
). Report is 8 commits behind head on master.:exclamation: Current head 2549a1e differs from pull request most recent head 5ff77f7
Please upload reports for the commit 5ff77f7 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
server/command.go | 9.90% | 100 Missing :warning: |
server/http.go | 76.82% | 15 Missing and 4 partials :warning: |
server/store.go | 57.14% | 12 Missing and 3 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@ayusht2810 Please resolve conflicts
@mickmister Gentle reminder for review
@mickmister Gentle reminder to review the PR
LGTM 👍 just some non-blocking comments for discussion
For the markdown table in the
channel-settings list
response, we should probably have it say:Default: Allow meetings in public channels, private channels, and DMs/GMs
or
Default: Allow meetings only in private channels and DMs/GMs
then simply omit any rows from the table that have the "default" value. We could probably just remove that channel from the saved settings map if the user chooses "default". 0/5 on this though
@mickmister the default setting has a dynamic value (i.e. it can be updated in plugin configuration) Didn't get you on this, please explain.
@mickmister the default setting has a dynamic value (i.e. it can be updated in plugin configuration) Didn't get you on this, please explain.
@Kshitij-Katiyar Yes I was suggesting we display this value in the response in the wording I posted above. Just so when someone wants to inspect the settings on a channel, they can know what the default value is. Then we can omit any channels that are either unassigned or using the default value
Default: Allow meetings only in private channels and DMs/GMs
this was the existing channel setting list
And this is the updated channel setting list
@mickmister I hope this aligns with your suggestions. Please let us know if anything else is required.
@fmartingr Gentle reminder for review
@mickmister Fixed the review comments added by you
@AayushChaudhary0001 Fixed the above use case. Can you please re-test the PR?
@wiggin77 Can you please review this PR. Its already approved but we need a codeowner's approval for merging
@wiggin77 Fixed the review comments added by you, please re-review
@wiggin77 Updated. Please re-review
Summary
Ticket Link
79
Screenshots
What to test?
settings
slash command when posting meeting links is restricted.