Hide participants from other regforms #6176
Conversation
| @@ -18,6 +18,7 @@ | |||
| {% endcall %} | |||
| {% if regforms %} | |||
| {% call form_fieldset(_('Registration forms')) %} | |||
| {{ form_row(form.hide_participants_from_other_forms) }} | |||
There was a problem hiding this comment.
right now the toggle is below the save button, which is not ideal. we also don't have a perfect solution for this, but I think if you place the save button just below this toggle it would make more sense
There was a problem hiding this comment.
one last thing here - it's still not ideal but we think it looks better if this input goes under the regform table
(we will probably do some UI improvements here on a later PR anyway, but if you could switch these two around here it'd be nice)
|
Overall the changes look good! You can go ahead and fix the tests (and maybe add some to test this new behavior). When you're done let me know so I can take a closer look / test the PR |
|
I fixed privacy tests and added some for the new feature, please check. |
| """Check whether the user is registered in the event. | ||
|
|
||
| If registration, return True only if user is registered in the same regform as registration. |
There was a problem hiding this comment.
please use the reST format for docstrings
| If registration, return True only if user is registered in the same regform as registration. | |
| :param registration: A registration. If defined, return True only if user is registered in the same regform as registration. | |
| ~Registration.is_deleted, | ||
| ~RegistrationForm.is_deleted)) | ||
| if registration: | ||
| query = query.filter(Registration.registration_form == registration.registration_form) |
There was a problem hiding this comment.
Actually, it seems you're only using the registration to get its regform.. Why not just pass the regform itself? It would also make the docstring a bit clearer
| registrations = sorted(_deduplicate_reg_data(_process_registration(reg, column_names) | ||
| for reg in query if reg.is_publishable(is_participant)), | ||
| for reg in query | ||
| if reg.is_publishable(user, | ||
| hide_participants_from_other_forms)), | ||
| key=lambda reg: tuple(x['text'].lower() for x in reg['columns'])) | ||
| return {'headers': headers, | ||
| 'rows': registrations, |
There was a problem hiding this comment.
this statement is starting to become a bit unreadable with all the identation. i'd suggest to format it like this:
registrations = _deduplicate_reg_data(_process_registration(reg, column_names)
for reg in query
if reg.is_publishable(user, hide_participants_from_other_forms))
return {'headers': headers,
'rows': sorted(registrations, key=lambda reg: tuple(x['text'].lower() for x in reg['columns'])),
'show_checkin': any(registration['checked_in'] for registration in registrations),
'num_participants': query.count()}(theres plenty of places where we do the sorting in the return statement, moreover the order doesn't matter for show_checkin)
| return db.or_(db.and_(is_participant, cls.registration_form.has(publish_registrations_participants=mode)), | ||
| db.and_(~is_participant, cls.registration_form.has(publish_registrations_public=mode))) |
There was a problem hiding this comment.
even though this works, it gets quite confusing when you're trying to track where is_participant comes from
for readability, i'd put the function definition after is_participant is defined.. or if that looks awkward, maybe pass it explicitly to the function
| query = query.filter(reg_alias.registration_form_id == cls.registration_form_id) | ||
| is_participant = query.exists() | ||
| else: | ||
| is_participant = db.text('false') |
There was a problem hiding this comment.
| is_participant = db.text('false') | |
| is_participant = literal(False) |
literal from sqlalchemy is preferred for this
| @pytest.mark.usefixtures('dummy_reg') | ||
| def test_registration_visibility(dummy_event, dummy_regform): | ||
| def test_registration_visibility(dummy_event, dummy_regform, dummy_user): |
There was a problem hiding this comment.
optional, but it might be a good idea to parametrize this test, specially if you're adding more test cases
Request to add new privacy setting for Participant List: setting to hide participants from other registration forms.
This PR replaces #6167, the code is done following previous review from Duarte.
Note: I will fix the privacy_test.py once we agree on the code change, thanks.