Skip to content

Issue/createroom api behavior does not match spec #3562

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

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

Conversation

tristianc
Copy link

Pull Request Checklist

Signed-off-by: Tristian Celestin <[email protected]>

@tristianc tristianc requested a review from a team as a code owner April 20, 2025 14:53
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2025

CLA assistant check
All committers have signed the CLA.

guestsCanJoin = true
case "public":
joinRuleContent.JoinRule = spec.Public
historyVisibilityContent.HistoryVisibility = historyVisibilityShared
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer it if you just set the createRequest.StatePreset rather than re-implement the state preset code here.

i.e:

// If unspecified, the server should use the visibility
// to determine which preset to use.
// A visibility of public equates to a preset of public_chat
// and private visibility equates to a preset of private_chat.
if createRequest.StatePreset == "" {
    switch createRequest.Visibility {
        // Rooms default to private visibility if this key is not included.
        case "private", "":
            createRequest.StatePreset = "private_chat"
        case "public":
            createRequest.StatePreset = "public_chat"
    }
}

switch createRequest.StatePreset {
// ... rest of code

Copy link
Contributor

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

tristianc added 2 commits May 18, 2025 22:33
Instead, if the StatePreset is unset, set it based on the Visibility.
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.

3 participants