TF-4301 Address code review comments on label feature
- Make LabelModalMixin interactor getters non-nullable; remove force-unwraps at call sites - Show positiveLabel and add semanticLabel in progressing button for accessibility - Use timestamp-based label name in E2E scenario to prevent flakiness on reused accounts - Guard labelAs action with isLabelAvailable check in SingleEmailController - Don't pop back on label creation failure so users can retry without losing context - Block modal dismissal while label creation request is in flight - Null-guard email.id before toggling label in ThreadView::onCreateANewLabelAction - Log stackTrace and rethrow in DialogRouter to distinguish errors from user cancel
This commit is contained in:
@@ -55,14 +55,17 @@ class ModalListActionButtonWidget extends StatelessWidget {
|
|||||||
),
|
),
|
||||||
);
|
);
|
||||||
|
|
||||||
Widget progressingButton = const SizedBox(
|
Widget progressingButton = SizedBox(
|
||||||
height: 48,
|
height: 48,
|
||||||
width: 153,
|
width: 153,
|
||||||
child: ConfirmDialogButton(
|
child: ConfirmDialogButton(
|
||||||
label: '',
|
label: positiveLabel,
|
||||||
padding: EdgeInsets.zero,
|
padding: EdgeInsets.zero,
|
||||||
backgroundColor: AppColor.primaryMain,
|
backgroundColor: AppColor.primaryMain,
|
||||||
child: CupertinoLoadingWidget(color: Colors.white),
|
child: CupertinoLoadingWidget(
|
||||||
|
color: Colors.white,
|
||||||
|
semanticLabel: positiveLabel,
|
||||||
|
),
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|||||||
@@ -49,7 +49,7 @@ class CreateANewTagFromAnEmailScenario extends BaseTestScenario
|
|||||||
await addLabelModalRobot.tapCreateANewLabel();
|
await addLabelModalRobot.tapCreateANewLabel();
|
||||||
await _expectCreateNewLabelModalVisible();
|
await _expectCreateNewLabelModalVisible();
|
||||||
|
|
||||||
const newLabelName = 'New Label 1';
|
final newLabelName = 'New Label ${DateTime.now().microsecondsSinceEpoch}';
|
||||||
await createLabelModalRobot.enterNewLabelName(newLabelName);
|
await createLabelModalRobot.enterNewLabelName(newLabelName);
|
||||||
await createLabelModalRobot.tapPositiveActionButton(LabelActionType.create);
|
await createLabelModalRobot.tapPositiveActionButton(LabelActionType.create);
|
||||||
await _expectLabelAsToEmailToastMessageSuccessVisible(
|
await _expectLabelAsToEmailToastMessageSuccessVisible(
|
||||||
|
|||||||
@@ -952,6 +952,9 @@ class SingleEmailController extends BaseController with AppLoaderMixin {
|
|||||||
pressEmailAction(actionType, presentationEmail);
|
pressEmailAction(actionType, presentationEmail);
|
||||||
break;
|
break;
|
||||||
case EmailActionType.labelAs:
|
case EmailActionType.labelAs:
|
||||||
|
if (!isLabelAvailable) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
mailboxDashBoardController.openAddLabelToEmailDialogModal(
|
mailboxDashBoardController.openAddLabelToEmailDialogModal(
|
||||||
email: presentationEmail,
|
email: presentationEmail,
|
||||||
onCreateANewLabelAction: () {
|
onCreateANewLabelAction: () {
|
||||||
|
|||||||
@@ -107,10 +107,10 @@ class LabelController extends BaseController
|
|||||||
}
|
}
|
||||||
|
|
||||||
@override
|
@override
|
||||||
CreateNewLabelInteractor? get createNewLabelInteractor => _createNewLabelInteractor;
|
CreateNewLabelInteractor get createNewLabelInteractor => _createNewLabelInteractor!;
|
||||||
|
|
||||||
@override
|
@override
|
||||||
EditLabelInteractor? get editLabelInteractor => _editLabelInteractor;
|
EditLabelInteractor get editLabelInteractor => _editLabelInteractor!;
|
||||||
|
|
||||||
DeleteALabelInteractor? get deleteALabelInteractor => _deleteALabelInteractor;
|
DeleteALabelInteractor? get deleteALabelInteractor => _deleteALabelInteractor;
|
||||||
|
|
||||||
@@ -221,9 +221,6 @@ class LabelController extends BaseController
|
|||||||
bool shouldPop = false,
|
bool shouldPop = false,
|
||||||
}) {
|
}) {
|
||||||
toastManager.showMessageFailure(failure);
|
toastManager.showMessageFailure(failure);
|
||||||
if (shouldPop) {
|
|
||||||
popBack();
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
void _addLabelToList(Label newLabel) {
|
void _addLabelToList(Label newLabel) {
|
||||||
|
|||||||
@@ -12,8 +12,8 @@ import 'package:tmail_ui_user/main/routes/dialog_router.dart';
|
|||||||
mixin LabelModalMixin {
|
mixin LabelModalMixin {
|
||||||
List<Label> get labels;
|
List<Label> get labels;
|
||||||
ImagePaths get imagePaths;
|
ImagePaths get imagePaths;
|
||||||
CreateNewLabelInteractor? get createNewLabelInteractor;
|
CreateNewLabelInteractor get createNewLabelInteractor;
|
||||||
EditLabelInteractor? get editLabelInteractor;
|
EditLabelInteractor get editLabelInteractor;
|
||||||
|
|
||||||
Future<dynamic> openCreateNewLabelModal({
|
Future<dynamic> openCreateNewLabelModal({
|
||||||
required AccountId accountId,
|
required AccountId accountId,
|
||||||
@@ -26,8 +26,8 @@ mixin LabelModalMixin {
|
|||||||
accountId: accountId,
|
accountId: accountId,
|
||||||
imagePaths: imagePaths,
|
imagePaths: imagePaths,
|
||||||
verifyNameInteractor: verifyNameInteractor,
|
verifyNameInteractor: verifyNameInteractor,
|
||||||
createNewLabelInteractor: createNewLabelInteractor!,
|
createNewLabelInteractor: createNewLabelInteractor,
|
||||||
editLabelInteractor: editLabelInteractor!,
|
editLabelInteractor: editLabelInteractor,
|
||||||
),
|
),
|
||||||
dialogLabel: 'create-new-label-modal',
|
dialogLabel: 'create-new-label-modal',
|
||||||
);
|
);
|
||||||
@@ -47,8 +47,8 @@ mixin LabelModalMixin {
|
|||||||
selectedLabel: selectedLabel,
|
selectedLabel: selectedLabel,
|
||||||
actionType: LabelActionType.edit,
|
actionType: LabelActionType.edit,
|
||||||
verifyNameInteractor: verifyNameInteractor,
|
verifyNameInteractor: verifyNameInteractor,
|
||||||
createNewLabelInteractor: createNewLabelInteractor!,
|
createNewLabelInteractor: createNewLabelInteractor,
|
||||||
editLabelInteractor: editLabelInteractor!,
|
editLabelInteractor: editLabelInteractor,
|
||||||
),
|
),
|
||||||
dialogLabel: 'edit-label-modal',
|
dialogLabel: 'edit-label-modal',
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -422,6 +422,9 @@ class _CreateNewLabelModalState extends State<CreateNewLabelModal> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
void _onCloseModal() {
|
void _onCloseModal() {
|
||||||
|
if (_createLabelStateNotifier.value == LabelPositiveButtonState.progressing) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
_clearInputFocus();
|
_clearInputFocus();
|
||||||
popBack();
|
popBack();
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -651,11 +651,16 @@ class ThreadView extends GetWidget<ThreadController>
|
|||||||
dashboardController.toggleLabelToEmail(emailId, label, isSelected);
|
dashboardController.toggleLabelToEmail(emailId, label, isSelected);
|
||||||
},
|
},
|
||||||
onCreateANewLabelAction: () {
|
onCreateANewLabelAction: () {
|
||||||
|
final emailId = email.id;
|
||||||
|
if (emailId == null) {
|
||||||
|
logWarning('ThreadView::onCreateANewLabelAction: Email id is null');
|
||||||
|
return;
|
||||||
|
}
|
||||||
dashboardController.labelController.handleLabelActionType(
|
dashboardController.labelController.handleLabelActionType(
|
||||||
actionType: LabelActionType.create,
|
actionType: LabelActionType.create,
|
||||||
accountId: dashboardController.accountId.value,
|
accountId: dashboardController.accountId.value,
|
||||||
onLabelActionCallback: (label) =>
|
onLabelActionCallback: (label) =>
|
||||||
dashboardController.toggleLabelToEmail(email.id!, label, true),
|
dashboardController.toggleLabelToEmail(emailId, label, true),
|
||||||
);
|
);
|
||||||
},
|
},
|
||||||
),
|
),
|
||||||
|
|||||||
@@ -41,8 +41,9 @@ class DialogRouter {
|
|||||||
);
|
);
|
||||||
|
|
||||||
return returnedValue;
|
return returnedValue;
|
||||||
} catch (e) {
|
} catch (e, stackTrace) {
|
||||||
logWarning('DialogRouter::pushGeneralDialog: Exception = $e');
|
logWarning('DialogRouter::pushGeneralDialog: Exception = $e\n$stackTrace');
|
||||||
|
rethrow;
|
||||||
} finally {
|
} finally {
|
||||||
if (PlatformInfo.isWeb) {
|
if (PlatformInfo.isWeb) {
|
||||||
_isMapDialogOpened.remove(routeName);
|
_isMapDialogOpened.remove(routeName);
|
||||||
@@ -112,8 +113,9 @@ class DialogRouter {
|
|||||||
barrierLabel: dialogLabel,
|
barrierLabel: dialogLabel,
|
||||||
pageBuilder: (_, __, ___) => child,
|
pageBuilder: (_, __, ___) => child,
|
||||||
);
|
);
|
||||||
} catch (e) {
|
} catch (e, stackTrace) {
|
||||||
logWarning('DialogRouter::openDialogModal: Exception = $e');
|
logWarning('DialogRouter::openDialogModal: Exception = $e\n$stackTrace');
|
||||||
|
rethrow;
|
||||||
} finally {
|
} finally {
|
||||||
if (PlatformInfo.isWeb && dialogLabel != null) {
|
if (PlatformInfo.isWeb && dialogLabel != null) {
|
||||||
_isMapDialogOpened.remove(dialogLabel);
|
_isMapDialogOpened.remove(dialogLabel);
|
||||||
|
|||||||
Reference in New Issue
Block a user