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.

Sign up for GitHub

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

Jump to bottom

feat: Add chat/workspaces with discord map #215

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

feat: Add chat/workspaces with discord map #215

wants to merge 25 commits into from

Conversation

martinalbert
Copy link
Contributor

Description

Adds new profile chat/workspaces with discord map.

Motivation and Context

Need of workspace id in chat/channels usecases.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING document.
  • I haven't repeated the code. (DRY)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@martinalbert martinalbert marked this pull request as ready for review March 15, 2022 16:25
janhalama
janhalama reviewed Mar 22, 2022
View reviewed changes
Copy link
Contributor

@janhalama janhalama left a comment

Choose a reason for hiding this comment

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

Looks good to me, I found just one issue in the rate limit mapping.

janhalama
janhalama approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@janhalama janhalama left a comment

Choose a reason for hiding this comment

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

LGTM

jnv
jnv reviewed Mar 25, 2022
View reviewed changes
Copy link
Contributor

@jnv jnv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have only non-blocking suggestions, feel free to merge this if you prefer to improve the docs separately.

grid/chat/workspaces/profile.supr Outdated Show resolved Hide resolved
grid/chat/workspaces/profile.supr Outdated
input {
"""
As Bot
Boolean representing whether to authorize as bot
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find this explanation very helpful. Also it's not clear to me why this is needed here since all other maps authorize the user as bot...

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of this field is limited, so pick this suggestion with grain of salt. My issues with this comment:

  • the fact that it's boolean is redundant,
  • it's not clear what the default behavior is,
  • it's not clear what's the impact.
Suggested change
Boolean representing whether to authorize as bot
Whether the access token authorizes a user or bot. By default it authorizes a user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jnv I'm not even sure why I added this field when we use bot auth everywhere. Probably to start discussion whether to include it in other usecases as well. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to remove this field? Really, this should be an integration parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll remove it.

grid/chat/workspaces/profile.supr Outdated

"""
Icon
Icon of the retrieved workspace
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it URL or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Icon hash, I'll edit the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is icon hash and how do I use it? IMO only an image URL would make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I checked and icon hash can be used to construct URL.

grid/chat/workspaces/profile.supr Outdated
Name
Name of the retrieved workspace
"""
name string
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be optional?

grid/chat/workspaces/profile.supr
}
}

model Workspace {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the workspace include some URL or something? Slack definitely has those and Discord servers have AFAIK vanity URLs (and if it's missing, I think the URL can be easily composed with server ID).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add optional field for it 👍🏻

@jnv jnv self-assigned this Apr 12, 2022
@martinalbert martinalbert requested a review from jnv June 9, 2022 10:58
jnv
jnv suggested changes Jun 10, 2022
View reviewed changes
Copy link
Contributor

@jnv jnv left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I still have issue with asBot input, tell me if you see it as must-have.

grid/chat/workspaces/profile.supr Outdated
input {
"""
As Bot
Boolean representing whether to authorize as bot
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to remove this field? Really, this should be an integration parameter.

@martinalbert
Copy link
Contributor Author

@jnv it makes more sense to have it as integration parameter 👍🏻

@martinalbert
chore(chat/workspaces): update profile
b00e525 
- remove empty input for passing lint
@tmladek
Copy link
Contributor

tmladek commented Dec 6, 2022

What's the status on this, can this be merged?

@martinalbert
Copy link
Contributor Author

Did not get approve from Honza, so I'm not sure, should be ready for final review.

@jnv jnv removed their assignment Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnv jnv jnv requested changes

@janhalama janhalama janhalama approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinalbert @tmladek @jnv @janhalama