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

Invalid group name #29

Closed
mizi opened this issue Jan 17, 2017 · 4 comments · Fixed by #33
Closed

Invalid group name #29

mizi opened this issue Jan 17, 2017 · 4 comments · Fixed by #33

Comments

@mizi
Copy link

mizi commented Jan 17, 2017

The last in ws_disconnect causes the following exception:

TypeError: Group name must be a valid unicode string containing only alphanumerics, hyphens, or periods.

because settings.NOTIFICATION_CHANNEL is a format string.

I think this line isn't needed and was left there by mistake in 251bfc0

@channel_session_user
def ws_disconnect(message):
    """
    Connected to websocket.disconnect
    """
    logger.debug("Removing connection for user {} (disconnect)".format(message.user))
    for subscription in models.Subscription.objects.filter(settings__user=message.user):
        Group(
            settings.NOTIFICATION_CHANNEL.format(
                notification_key=subscription.notification_type.key
            )
        ).discard(message.reply_channel)
    Group(settings.NOTIFICATION_CHANNEL).discard(message.reply_channel)
@benjaoming
Copy link
Member

The group is supposed to contain the notification key such that people can subscribe to only specific channels (notifications).

That should be fine, however there could be an issue with your notification key - could we somehow allow notification keys that don't conform to only alphanumerics, hyphens, or periods. ?

@ztomaz
Copy link
Contributor

ztomaz commented May 17, 2017

The problem is the second Group(...).discard function:
Group(settings.NOTIFICATION_CHANNEL).discard(message.reply_channel), because it uses non formated settings.NOTIFICATION_CHANNEL (value is: "nyt_all-{notification_key:s}") which contains non allowed characters. I also think it was left there by mistake

ztomaz referenced this issue in ztomaz/django-nyt May 17, 2017
@benjaoming
Copy link
Member

I totally missed the point in this issue, thanks @ztomaz :)

@mizi
Copy link
Author

mizi commented May 26, 2017

Thanks @benjaoming, thanks @ztomaz :)

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 a pull request may close this issue.

3 participants