Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

fix test_auto_create_auto_join_where_no_consent #4886

Merged
merged 9 commits into from
Mar 19, 2019

Conversation

neilisfragile
Copy link
Contributor

No description provided.

a sensible thing to test for, and (b) it doesn't work anyway because you can't
change the config after the EventCreationHandler has been instantiated.
"""Test to ensure that the first user is not auto-joined to a room if
they have not given general consent.
"""
self.hs.config.user_consent_at_registration = True
Copy link
Member

Choose a reason for hiding this comment

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

does this and the next line actually do anything? I'd expect them to be redundant now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope

# * The server is configured to auto-join to a room
# (and autocreate if necessary)
event_creation_handler = self.hs.get_event_creation_handler()
event_creation_handler._block_events_without_consent_error = (
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit dubious about this poking into the guts of EventCreationHandler - it looks fragile. OTOH I can't really think of a better way of testing it, other than creating a new test class (possibly a subclass of RegistrationTestCase) which uses a different config - which might be a bit fiddly right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this really mings - I'll comment explicitly to make it clearer

room_alias_str = "#room:test"
self.hs.config.auto_join_rooms = [room_alias_str]

# When the user is registered, and post consent actions are called
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to make sense?

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've tried to make it clearer with formatting

@richvdh richvdh changed the base branch from rav/broken_registration_test to develop March 19, 2019 10:30
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #4886 into develop will decrease coverage by 0.18%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #4886      +/-   ##
===========================================
- Coverage    77.87%   77.68%   -0.19%     
===========================================
  Files          326      326              
  Lines        33949    34620     +671     
  Branches      5597     5811     +214     
===========================================
+ Hits         26437    26896     +459     
- Misses        5902     6068     +166     
- Partials      1610     1656      +46

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

can you rename the changelog to be a .misc; it's not a user-facing bug

@richvdh
Copy link
Member

richvdh commented Mar 19, 2019

(lgtm other than the changelog)

@richvdh richvdh merged commit 88f0675 into develop Mar 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants