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

Fix: when a attendee is a second person without email, skip alreadyInvitedEmails check #2977

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

JackyW83
Copy link

@JackyW83 JackyW83 commented Mar 26, 2021

In case if multiple attendees have empty email, the second one won't show in search result by
block of alreadyInvitedEmails. Add email is not empty check in findAttendeesFromContactsAPI

@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 23.67%. Comparing base (2eaf63a) to head (58fb57b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2977   +/-   ##
=========================================
  Coverage     23.67%   23.67%           
  Complexity      457      457           
=========================================
  Files           249      249           
  Lines         11774    11774           
  Branches       2214     2204   -10     
=========================================
  Hits           2788     2788           
- Misses         8668     8669    +1     
+ Partials        318      317    -1     
Flag Coverage Δ
javascript 15.25% <ø> (ø)
php 59.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -186,7 +186,7 @@ export default {
name = email
}

if (this.alreadyInvitedEmails.includes(email)) {
if (this.alreadyInvitedEmails.includes(email) && email) {
Copy link
Member

Choose a reason for hiding this comment

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

but now it skips the attendee if it has an email, right?

Copy link
Author

@JackyW83 JackyW83 Apr 6, 2021

Choose a reason for hiding this comment

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

No. If user has email, he/she only skip when this.alreadyInvitedEmails.includes(email) === true, and skip all users don't have email.
I updated the condition check order in latest commit, maybe this looks more intuitive.

 if (email && this.alreadyInvitedEmails.includes(email)) {
	return
 }

In country like China, a lot of people don't have email, so updated this :)

@ChristophWurst
Copy link
Member

ChristophWurst commented Mar 6, 2023

Squash your commits into a single one and rebase to latest main done

Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Logic sounds good, didn't test

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews bug 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 31, 2024
@miaulalala miaulalala added this to the v5.0.0 milestone Jul 31, 2024
@miaulalala
Copy link
Contributor

/backport to stable4.7

@backportbot backportbot bot added the backport-request A backport was requested for this pull request label Jul 31, 2024
@miaulalala miaulalala marked this pull request as draft July 31, 2024 08:47
@miaulalala miaulalala marked this pull request as ready for review July 31, 2024 08:47
@ChristophWurst ChristophWurst merged commit 1ec43f2 into nextcloud:main Jul 31, 2024
28 checks passed
@backportbot backportbot bot removed the backport-request A backport was requested for this pull request label Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants