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

Enhance validation and check for bad/no recipients in PMs #937

Merged
merged 5 commits into from Jul 3, 2021

Conversation

prah23
Copy link
Member

@prah23 prah23 commented Feb 27, 2021

This PR adds a commit that refactors the get_invalid_recipients_emails() method
of the model to check_recipient_validity() which checks if the
recipient's name and email match.

This commit follows this strategy:

  • Ensures that the email and the name are of the same user.
  • Removes extra text after the email.

The recipients' information is also tidied for all the valid recipients,
by formatting the information to fit the name <email> format.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@prah23 Thanks for the bugfix and looking into the PM validation 👍

Some comments inline, but also feel free to chat in the stream.

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
zulipterminal/model.py Outdated Show resolved Hide resolved
zulipterminal/model.py Outdated Show resolved Hide resolved
@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 28, 2021
@neiljp neiljp added this to the Next Release milestone Feb 28, 2021
@prah23 prah23 force-pushed the pm_recipients_validation branch 2 times, most recently from 2ee89e1 to 6e66b31 Compare March 9, 2021 21:31
Copy link
Member

@zee-bit zee-bit left a comment

Choose a reason for hiding this comment

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

Thanks for working on it.
This looks good and works as expected. 👍

I suppose tests would be added after some more review and feedback?

tests/ui_tools/test_boxes.py Outdated Show resolved Hide resolved
@prah23
Copy link
Member Author

prah23 commented Mar 11, 2021

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"

@zulipbot zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Mar 11, 2021
@prah23 prah23 force-pushed the pm_recipients_validation branch 3 times, most recently from 6ced44d to b38054a Compare March 13, 2021 23:02
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Mar 14, 2021
@zulipbot zulipbot added size: XL and removed size: L labels Mar 16, 2021
@prah23 prah23 force-pushed the pm_recipients_validation branch 4 times, most recently from 96c792c to 327723d Compare March 18, 2021 22:04
@prah23
Copy link
Member Author

prah23 commented Mar 19, 2021

@zulipbot remove "PR awaiting update"

@zulipbot zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Mar 19, 2021
@prah23
Copy link
Member Author

prah23 commented Mar 22, 2021

@zulipbot add "PR needs review"

@prah23 prah23 added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 1, 2021
@prah23
Copy link
Member Author

prah23 commented Jul 1, 2021

Thanks for the review, @neiljp!
I've restructured the commits as suggested, and accommodated the inline changes suggested.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@prah23 Thanks for responding to the minor points, and the restructure is a good first step - see if you can identify the others I pointed at 👍

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/boxes.py Show resolved Hide resolved
zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 2, 2021
@prah23 prah23 force-pushed the pm_recipients_validation branch 3 times, most recently from bbe1581 to 70e15f9 Compare July 2, 2021 15:41
@prah23 prah23 added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 2, 2021
@prah23
Copy link
Member Author

prah23 commented Jul 2, 2021

Thanks for your patience on this, @neiljp!
I've added another commit to extract validation logic to a helper, and the thorough validation is a commit after that, which changes the behavior inside the helper. The extension of the validation to SEND_MESSAGE and SAVE_AS_DRAFT is pushed to another commit as well.

Previously, a list of emails were expected as a parameter in the
model helper `get_invalid_recipient_emails` which has been refactored
to `is_valid_private_recipient` to abstract the processing of the list
to `boxes.py` and rely on the model helper to verify each recipient
individually.

The model helper checks whether a given recipient email exists in the
`user_dict` and returns the relevant boolean.
This commit extracts the current private message recipient validation
into a helper method, `_validate_recipients_and_notify_invalid_ones`,
that returns a bool, indicating whether or not, all of the recipient
emails mentioned in the `to_write_box` are valid (returns False even on
a single invalid email). The `CYCLE_COMPOSE_FOCUS` keypress now relies
on this helper method to specify whether or not to continue with it's
flow.
This commit modifies the validation helper method to assist with
a more thorough validation of the recipients in the `to_write_box`.
The `_tidy_valid_recipients_and_notify_invalid_ones` helper is used
to extract individual recipients, discard any unnecessary text after
the email and then call the `is_valid_private_recipient` helper
from the model.

The `is_valid_private_recipient` helper in `model.py` now also
ensures that the name of the recipient extracted matches the
corresponding name in the `user_dict`.

Tests added.
Prior to this commit, private message recipients were only validated on
`CYCLE_COMPOSE_FOCUS` keypresses. This commit extends the validation
process to `SEND_MESSAGE` and `SAVE_AS_DRAFT` keypresses as well, to
ensure valid recipients before any meaningful action occurs in the
compose box.

Tests added.
This commit modifies the `invalid_recipients_error` message to also
mention the keys used for autocompleting recipients, so the user is
able to quickly reach the valid form of the intended recipients.

Tests updated.
@neiljp neiljp added PR soon to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels Jul 3, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jul 3, 2021

@prah23 Thanks for the restructure - this has cleaner smaller commits now 👍

I just removed a duplicate assignment in the last commit and pushed to the PR before merging 🎉

@neiljp neiljp merged commit e42100f into zulip:main Jul 3, 2021
@prah23
Copy link
Member Author

prah23 commented Jul 3, 2021

Thanks for your patience!

@neiljp neiljp added enhancement New feature or request area: UI General user interface update labels Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UI General user interface update enhancement New feature or request PR soon to be merged PR has been reviewed & is ready to be merged size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants