feat: Add schedule send #2143
feat: Add schedule send #2143
Conversation
TommyDL-Infomaniak
added
the
feature
TommyDL-Infomaniak
force-pushed
the
ft/schedule-send
branch
from
8a2a628 to
08ba7b2
Compare
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
from
6eddfcd to
e95ebf1
Compare
|
This PR/issue depends on:
|
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
4 times, most recently
from
55e8b34 to
2a8156c
Compare
app/src/main/java/com/infomaniak/mail/data/api/ApiRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/infomaniak/mail/data/api/ApiRepository.kt
Outdated
Show resolved
Hide resolved
| <string name="buttonModify">Modificare</string> | ||
| <string name="buttonMore">Altro</string> | ||
| <string name="buttonMoreStorage">Ottieni più spazio di archiviazione</string> | ||
| <string name="buttonNewMessage">Nuovo messaggio</string> | ||
| <string name="buttonNo">No</string> | ||
| <string name="buttonOpenMyCalendar">Apri nel mio calendario</string> | ||
| <string name="buttonRefuse">Rifiuta</string> | ||
| <string name="buttonRequestPassword">Richiesta password</string> | ||
| <string name="buttonReschedule">Riprogrammare</string> |
There was a problem hiding this comment.
Check the italian strings with @valentinperignon because I think there are a few mistakes here and there
| @@ -403,6 +427,9 @@ | |||
| <string name="reviewAlertTitle">Aimez-vous Infomaniak Mail ?</string> | |||
| <string name="saveToDriveDeviceItem">Enregistrer sur l’appareil</string> | |||
| <string name="saveToDriveItem">Enregistrer dans kDrive</string> | |||
| <string name="scheduleCancelled">Programmation annulée.</string> | |||
There was a problem hiding this comment.
Is this final point needed?
There was a problem hiding this comment.
This string and the editSendDescription string existed before as one, but we've split them up due to an inconsistency in the UI/UX flow.
In fact, it did not make sense to say it was cancelled while in reality it was not yet cancelled.
This is still an ongoing discussion.
| @@ -327,6 +347,8 @@ | |||
| <string name="messageIsDraftOption">Borrador</string> | |||
| <string name="messageReplyHeader">El %1$s, %2$s escribió:</string> | |||
| <string name="messageShowQuotedText">Mostrar la conversación</string> | |||
| <string name="mondayAfternoon">lunes por la tarde</string> | |||
There was a problem hiding this comment.
Why do you have a string unlike the other ones with a lowercase?
| <string name="buttonRestoreEmails">Restaurar correos electrónicos</string> | ||
| <string name="buttonSchedule">Programar un correo electrónico para enviarlo más tarde</string> | ||
| <string name="buttonScheduleTitle">Cronograma</string> |
There was a problem hiding this comment.
"Cronograma" means "Calendar"
| <string name="buttonRestoreEmails">E-Mails wiederherstellen</string> | ||
| <string name="buttonSchedule">Planen Sie den Versand von E-Mails für einen späteren Zeitpunkt</string> | ||
| <string name="buttonScheduleTitle">Zeitplan</string> |
There was a problem hiding this comment.
"Zeitplan" means "Calendar" as well. Fix this translation in italian as well if there is an issue there
| @@ -133,6 +133,7 @@ | |||
| <string name="buttonCreate">Erstellen</string> | |||
| <string name="buttonCreateAccount">Ein Konto erstellen</string> | |||
| <string name="buttonCreateFolder">Einen Ordner hinzufügen</string> | |||
| <string name="buttonCustomSchedule">Benutzerdefinierter Zeitplan</string> | |||
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
2 times, most recently
from
9d8fa57 to
3c2c55c
Compare
| if (newMessageViewModel.currentMailbox.featureFlags.contains(FeatureFlag.SCHEDULE_SEND_DRAFT)) { | ||
| binding.scheduleSendButton.isVisible = true | ||
| } |
There was a problem hiding this comment.
Don't hardcode the visibility once forever, you need to listen to a flow the same way the AI feature flag is listened to for when the feature flags get updated
|
|
||
| title.setText(R.string.disabledFeatureFlagTitle) | ||
| description.text = getString(R.string.disabledFeatureFlagDescription) | ||
| infoIllustration.setBackgroundResource(R.drawable.ic_update_logo) |
There was a problem hiding this comment.
The illustration doesn't seem to be the correct one
There was a problem hiding this comment.
This bottom sheet is too tightly coupled with the NewTransferViewModel and maybe LocalSettings. But we don't have that much time so it can be merged this way and I will decouple it before doing snooze to be able reuse it for the snooze feature
| LATER_THIS_MORNING( | ||
| R.string.laterThisMorning, | ||
| R.drawable.ic_morning_sunrise_schedule, | ||
| Date().getMorning(), | ||
| listOf(TimeToDisplay.NIGHT), | ||
| "laterThisMorning", | ||
| ), | ||
| MONDAY_MORNING( | ||
| R.string.mondayMorning, | ||
| R.drawable.ic_morning_schedule, | ||
| Date().getNextMonday().getMorning(), | ||
| listOf(TimeToDisplay.WEEKEND), | ||
| "nextMondayMorning", | ||
| ), | ||
| MONDAY_AFTERNOON( | ||
| R.string.mondayAfternoon, | ||
| R.drawable.ic_afternoon_schedule, | ||
| Date().getNextMonday().getAfternoon(), | ||
| listOf(TimeToDisplay.WEEKEND), | ||
| "nextMondayAfternoon", | ||
| ), | ||
| THIS_AFTERNOON( | ||
| R.string.thisAfternoon, | ||
| R.drawable.ic_afternoon_schedule, | ||
| Date().getAfternoon(), | ||
| listOf(TimeToDisplay.MORNING), | ||
| "thisAfternoon", | ||
| ), | ||
| THIS_EVENING( | ||
| R.string.thisEvening, | ||
| R.drawable.ic_evening_schedule, | ||
| Date().getEvening(), | ||
| listOf(TimeToDisplay.AFTERNOON), | ||
| "thisEvening", | ||
| ), | ||
| TOMORROW_MORNING( | ||
| R.string.tomorrowMorning, | ||
| R.drawable.ic_morning_schedule, | ||
| Date().tomorrow().getMorning(), | ||
| listOf(TimeToDisplay.NIGHT, TimeToDisplay.MORNING, TimeToDisplay.AFTERNOON, TimeToDisplay.EVENING), | ||
| "tomorrowMorning", | ||
| ), | ||
| NEXT_MONDAY( | ||
| R.string.nextMonday, | ||
| R.drawable.ic_arrow_return, | ||
| Date().getNextMonday().getMorning(), | ||
| listOf(TimeToDisplay.NIGHT, TimeToDisplay.MORNING, TimeToDisplay.AFTERNOON, TimeToDisplay.EVENING), | ||
| "nextMonday", | ||
| ) |
There was a problem hiding this comment.
Dates are computed a single time when the enum is instanciated this should be computed dynamically
| fun createScheduleItem(schedule: Schedule): ActionItemView = ActionItemView(this.context).apply { | ||
| setTitle(schedule.scheduleTitleRes) | ||
| setDescription(mostDetailedDate(context, date = schedule.date, format = FORMAT_DATE_DAY_MONTH)) | ||
| setIconResource(schedule.scheduleIconRes) | ||
| setClosingOnClickListener { | ||
| if (navigationArgs.isAlreadyScheduled) { | ||
| navigationArgs.draftResource?.let { | ||
| trackScheduleSendEvent(schedule.matomoValue) | ||
| mainViewModel.rescheduleDraft(draftResource = it, schedule.date) | ||
| } | ||
| } else { | ||
| trackScheduleSendEvent(schedule.matomoValue) | ||
| newMessageViewModel.setScheduleDate(schedule.date) | ||
| newMessageViewModel.triggerSendMessage() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This method doesn't need to be an inner method, it makes the onViewCreated heavier and harder to read for no reason
| enum class TimeToDisplay { | ||
| NIGHT, | ||
| MORNING, | ||
| AFTERNOON, | ||
| EVENING, | ||
| WEEKEND; | ||
|
|
||
| companion object { | ||
| fun getTimeToDisplayFromDate(): TimeToDisplay { | ||
| val now = Date() | ||
| val timeSlot = Date(now.time) | ||
| return if (now.isWeekend()) WEEKEND else when (now) { | ||
| in timeSlot.setHour(0).setMinute(0)..timeSlot.setHour(7).setMinute(54) -> NIGHT | ||
| in timeSlot.setHour(7).setMinute(55)..timeSlot.setHour(13).setMinute(54) -> MORNING | ||
| in timeSlot.setHour(13).setMinute(55)..timeSlot.setHour(17).setMinute(54) -> AFTERNOON | ||
| in timeSlot.setHour(17).setMinute(55)..timeSlot.setHour(23).setMinute(54) -> EVENING | ||
| else -> NIGHT | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be done in an easier way but I'll refactor all this with the snooze because we don't have much more time
| companion object { | ||
| const val MIN_SCHEDULE_DELAY_MINUTES = 5 | ||
| const val MAX_SCHEDULE_DELAY_YEARS = 10 | ||
| } |
There was a problem hiding this comment.
These constants are more often than not used in another file than this one. Would it not be better to write them where they'll mainly be used?
| mainViewModel.showSelectDateAndTimeForScheduleDialog() | ||
| } else { | ||
| newMessageViewModel.showSelectDateAndTimeForScheduleDialog() |
There was a problem hiding this comment.
Using the view models as side effects to trigger the date picker is part of the reasons why this class has high coupling. I think we have situations where we returned values from bottom sheet or dialogs to the caller so he decides what to do next
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
from
e4a8ae7 to
e8c6fad
Compare
| app:layout_constraintBottom_toBottomOf="@id/userName" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintStart_toEndOf="@+id/userName" | ||
| app:layout_constraintTop_toTopOf="@+id/userName" | ||
| app:layout_constraintStart_toEndOf="@id/userName" | ||
| app:layout_constraintTop_toTopOf="@id/userName" |
There was a problem hiding this comment.
If you want to remove a lot of badly formatted code like this, do it directly on master in another PR when your PR is already 2k+ lines
| <argument | ||
| android:name="isAlreadyScheduled" | ||
| android:defaultValue="false" | ||
| app:argType="boolean" /> | ||
| <argument | ||
| android:name="draftResource" | ||
| android:defaultValue="@null" | ||
| app:argType="string" | ||
| app:nullable="true" /> |
There was a problem hiding this comment.
These values probably shouln't be passed down to the bottom sheet if the bottom sheet needs to be reused elswhere. Only the caller needs to know about it and the bottom sheet must tell the caller what button was clicked and nothing more
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
4 times, most recently
from
29ae022 to
dba74e5
Compare
gradle/libs.versions.toml
Outdated
There was a problem hiding this comment.
Your PR doesn't target the correct branch
| <ImageView | ||
| android:id="@+id/scheduleSendIcon" | ||
| android:layout_width="@dimen/smallIconSize" | ||
| android:layout_height="@dimen/smallIconSize" | ||
| android:layout_marginHorizontal="@dimen/marginStandardVerySmall" | ||
| android:contentDescription="@string/contentDescriptionScheduleSendIcon" | ||
| android:src="@drawable/ic_scheduled_messages" | ||
| android:visibility="gone" | ||
| app:layout_constraintBottom_toBottomOf="@id/shortMessageDate" | ||
| app:layout_constraintEnd_toStartOf="@id/shortMessageDate" | ||
| app:layout_constraintStart_toEndOf="@id/iconsSpace" | ||
| app:layout_constraintTop_toTopOf="@id/shortMessageDate" | ||
| app:tint="@color/scheduledIconColor" | ||
| tools:visibility="visible" /> |
There was a problem hiding this comment.
The icon is not aligned vertically with the checkmark. I don't know if fixing it is complicated
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
6 times, most recently
from
f77ba3e to
d52e821
Compare
…viously scheduled Draft
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
from
4e4c172 to
11abdd9
Compare
| isSuccess() -> { | ||
| scheduleAction = data?.scheduleAction | ||
| realmActionOnDraft = deleteDraftCallback(draft) | ||
| refreshScheduledDraftsFolder() |
There was a problem hiding this comment.
Doesn't look right that we need to refresh a folder after executing a draft action.
It's supposed to already be handled somewhere else.
| realmActionOnDraft?.let(realmActionsOnDraft::add) | ||
|
|
||
| [email protected] = scheduleAction |
There was a problem hiding this comment.
I don't understand why we do that here, and it doesn't look right.
KevinBoulongne
force-pushed
the
ft/schedule-send
branch
from
11abdd9 to
c62f707
Compare
|
This feature is behind a feature flag, and lacks the ability to delete scheduled messages for now (due to how the API deletes scheduled messages).
Depends on Infomaniak/android-core#259