Skip to content

Enable sending messages to an unlimited number of recipients #2923

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
Merged
merged 7 commits into from
Oct 16, 2024
Merged

Enable sending messages to an unlimited number of recipients #2923

merged 7 commits into from
Oct 16, 2024

Conversation

Ahmed-Naguib93
Copy link
Contributor

@Ahmed-Naguib93 Ahmed-Naguib93 commented Oct 9, 2024 •
edited
Loading

refs: MBL-17936
affects: Student , Teacher
release note: Enabled sending messages to an unlimited number of recipients

Test Plane

  • Open a new message
  • Send a message to over 100 recipients.
  • Observe that the "Send Individually" toggle becomes disabled.

Note

  • If you send a reply message to over 100 recipients, you will receive an error. This is not an issue on our side but a backend issue.

Screenshots

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-11.at.14.18.38.mp4

Checklist

  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
BeforeAfter

@Ahmed-Naguib93
Enable sending messages to an unlimited number of recipients
9a33aa1 
refs:MBL-17936
affects: Teacher , Student
release note: Enable sending messages to an unlimited number of recipients

test plan:
@Ahmed-Naguib93
Add unit test
3f7f391 
refs:MBL-17936
affects:Teacher, Student
release note: Enabled to send message over 100 recipients

test plan:
@Ahmed-Naguib93 Ahmed-Naguib93 self-assigned this Oct 9, 2024
@inst-danger
Copy link
Contributor

inst-danger commented Oct 9, 2024 •
edited
Loading

Teacher Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Oct 9, 2024 •
edited
Loading

Student Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Oct 9, 2024 •
edited by jira bot
Loading

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

Enabled sending messages to an unlimited number of recipients

Affected Apps: Student, Teacher

MBL-17936

Coverage New % Master % Delta
Canvas iOS 91.18% 91.17% 0.01%
Core/Core/Planner/CalendarEvent/Model/API/PostCalendarEventRequest.swift 0% 0% 0%
Core/Core/Planner/CalendarEvent/View/CustomFrequencyComponents/OccurrencesCountInputDialog.swift 0% 0% 0%
Core/Core/Planner/CalendarEvent/Model/Helpers/RecurrenceRule+SelectionDescription.swift 43.33% 43.33% 0%
Core/Core/Grades/View/CustomSwipeAction.swift 0% 0% 0%

Generated by 🚫 dangerJS against 3d67b62

@Ahmed-Naguib93
Merge branch 'master' into bugfix/MBL-17936-Conversation-messages-wit…
29545b5 
…h-more-than-100-recipients-fail-to-send
@Ahmed-Naguib93
Fix swiftLint issue
799c3ad 
refs:MBL-17936
affects:Student , Teacher
release note: Enabled sending messages to an unlimited number of recipients

test plan:
rh12
rh12 requested changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

Few minor things, beside the change about the additional info text discussed in slack.

@Ahmed-Naguib93
Set hint message when send message over 100 recipients
6ce9b82 
refs: MBL-17936
affects:Student , Teacher
release note:Enabled sending messages to an unlimited number of recipients

test plan:
@Ahmed-Naguib93 Ahmed-Naguib93 requested a review from rh12 October 11, 2024 11:48
rh12
rh12 reviewed Oct 11, 2024
View reviewed changes
Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

QA + 1

Thanks for the changes!
Will approve once the UX change below is confirmed.

Core/Core/Inbox/ComposeMessage/View/ComposeMessageView.swift Outdated
@@ -307,6 +307,7 @@ public struct ComposeMessageView: View, ScreenViewTrackable {
.font(.regular16, lineHeight: .condensed)
.foregroundColor(.textDark)
.padding(.vertical, 12)
.frame(maxHeight: .infinity, alignment: .center)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've asked Marci about this change, will update with his answer.
Regardless, please highlight these kind of unrelated changes in the PR description, especially if those are user facing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i faced an issues when you set much recipients in the list the 'TO' button in the top and '+' in the middle, so i fixed it
@rh12

Copy link
Contributor

Choose a reason for hiding this comment

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

Marci confirmed it, that the "To" text should stay top aligned. Actually the "+" should also stay top aligned.
(This was probably not spotted in the previous PR)

Please revert the "To" change, and could you please fix the "+"? If so, please add that to the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks will do

@Ahmed-Naguib93
set add recipients button pinned at top
5950db2 
refs:MBL-17936
affects:Student , Teacher
release note:Enabled sending messages to an unlimited number of recipients

test plan:
rh12
rh12 previously approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

QA + 1

vargaat
vargaat reviewed Oct 15, 2024
View reviewed changes
Core/Core/Inbox/ComposeMessage/ViewModel/ComposeMessageViewModel.swift Outdated
@@ -103,6 +104,8 @@ final class ComposeMessageViewModel: ObservableObject {
private var hiddenMessage: String = ""
private var autoTeacherSelect: Bool = false
private var teacherOnly: Bool = false
private var sendIndividualToggleTemp: Bool = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a more telling name here? Like sendIndividualToggleLastValue? This variable is definitely not temporary.

@Ahmed-Naguib93
Rename sendIndividualToggleLastValue
3d67b62 
refs:MBL-17936
affects:Student , Teacher
release note:Enabled sending messages to an unlimited number of recipients

test plan:
vargaat
vargaat approved these changes Oct 16, 2024
View reviewed changes
Copy link
Collaborator

@vargaat vargaat left a comment

Choose a reason for hiding this comment

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

QA+1

@vargaat vargaat merged commit bbd7048 into master Oct 16, 2024
5 checks passed
@vargaat vargaat deleted the bugfix/MBL-17936-Conversation-messages-with-more-than-100-recipients-fail-to-send branch October 16, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rh12 rh12 rh12 approved these changes

@vargaat vargaat vargaat approved these changes

@szabinst szabinst Awaiting requested review from szabinst szabinst is a code owner

Assignees

@Ahmed-Naguib93 Ahmed-Naguib93

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ahmed-Naguib93 @inst-danger @rh12 @vargaat