Skip to content

Comments

Room passwords#5

Open
bkmaibach wants to merge 7 commits intomasterfrom
room-passwords
Open

Room passwords#5
bkmaibach wants to merge 7 commits intomasterfrom
room-passwords

Conversation

@bkmaibach
Copy link
Contributor

Add password protection for rooms

Copy link
Contributor

@staydecent staydecent left a comment

Choose a reason for hiding this comment

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

Nice work. Few small notes. Also, no need to go back, but in the future it's best to keep migrations limited. For instance, in the future, if you were to create a new field and then rename it, all in a single PR/branch, then the migrations should be run fresh when the code is finalized.

Instead of having 0005_room_password_hash.py and 0006_auto_20200520_1724.py it could have been just a single migration creating the field with the proper name of password.

This starts to matter more on long-lived projects, where migrations keep getting tacked on.

hashed_password = Room.objects.get(id=room_id).password
print('GOT HASHED PASSWORD ', hashed_password)
print('CHECKING PASSWORD ', password)
if hashed_password is '' or hashed_password is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this can be simplified as if not hased_password

Comment on lines +16 to +17
else:
validated_data['password'] = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

If you had null=True on the password field, then you could skip having to set this to an empty string. Fore future reference.


class MessageSerializer(serializers.Serializer):
command = serializers.ChoiceField(choices=['INIT_CHAT', 'FETCH_ENTRIES', 'NEW_ENTRY', 'ENTRIES'])
command = serializers.ChoiceField(choices=['INIT_CHAT', 'FETCH_ENTRIES', 'NEW_ENTRY'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if this should reference the constants on the ChatConsumer class?

choices=[ChatConsumer.INIT_CHAT, ChatConsumer.FETCH_ENTRIES, ...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah!

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.

2 participants