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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jul 3, 2021
Merged

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

merged 5 commits into from
Jul 3, 2021

Conversation

prah23
Copy link
Member

@prah23 prah23 commented Feb 27, 2021 •
edited
Loading

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.

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Feb 27, 2021
neiljp
neiljp reviewed Feb 28, 2021
View reviewed changes
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.

@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 from d70ea17 to a2e71fa Compare March 1, 2021 21:15
@prah23 prah23 force-pushed the pm_recipients_validation branch 2 times, most recently from 2ee89e1 to 6e66b31 Compare March 9, 2021 21:31
zee-bit
zee-bit suggested changes Mar 10, 2021
View reviewed changes
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?

@prah23 prah23 force-pushed the pm_recipients_validation branch from 6e66b31 to d540143 Compare March 11, 2021 05:21
@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
@prah23 prah23 mentioned this pull request Mar 15, 2021
Open
@prah23 prah23 force-pushed the pm_recipients_validation branch from b38054a to 02931f4 Compare March 16, 2021 15:39
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] 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 prah23 force-pushed the pm_recipients_validation branch from 327723d to c0b072a Compare March 22, 2021 16:02
@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.

@prah23 prah23 force-pushed the pm_recipients_validation branch from fbac681 to 11a20f1 Compare July 1, 2021 22:55
neiljp
neiljp reviewed Jul 2, 2021
View reviewed changes
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 👍

@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 prah23 force-pushed the pm_recipients_validation branch from 70e15f9 to dc6eeff Compare July 2, 2021 15:48
@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.

@prah23 prah23 force-pushed the pm_recipients_validation branch from dc6eeff to a247af1 Compare July 2, 2021 19:53
prah23 added 4 commits July 3, 2021 03:14
@prah23
refactor: boxes/model: Verify each email individually through model.
8dff238 
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.
@prah23
refactor: boxes: Extract current recipient validation into a helper.
300e9a6 
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.
@prah23
boxes: Validate private message recipients more thoroughly.
10f6d0b 
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.
@prah23
boxes: Validate recipients on SEND_MESSAGE and SAVE_AS_DRAFT keypresses.
0072986 
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.
@prah23 prah23 force-pushed the pm_recipients_validation branch from a247af1 to 7b75fca Compare July 2, 2021 21:51
@prah23 @neiljp
boxes: Inform the user to use autocomplete on invalid recipients.
26743a0 
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 force-pushed the pm_recipients_validation branch from 7b75fca to 26743a0 Compare July 3, 2021 06:52
@neiljp neiljp added PR ready 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!

@prah23 prah23 mentioned this pull request Jul 5, 2021
Merged
@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
Reviewers

@neiljp neiljp neiljp left review comments

@zee-bit zee-bit zee-bit requested changes

@Ezio-Sarthak Ezio-Sarthak Ezio-Sarthak requested changes

Assignees
No one assigned
Labels
area: UI General user interface update enhancement New feature or request PR ready to be merged PR has been reviewed & is ready to be merged size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@prah23 @neiljp @zee-bit @Ezio-Sarthak @zulipbot