SelfPrivacy
/
selfprivacy.org.app
10
7
Fork 5
Code Issues 77 Pull Requests 3 Projects 1 Releases 24 Wiki Activity

refactor(job): Implement polymorphic behavior on creation for jobs #134

Merged
NaiJi merged 3 commits from server-settings into master 2022-10-08 19:22:46 +03:00
Conversation 1 Commits 3 Files Changed 5
5 changed files with 57 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
lib/logic/cubit/client_jobs/client_jobs_cubit.dart
View File

@ -24,14 +24,13 @@ class JobsCubit extends Cubit<JobsState> {
final ServicesCubit servicesCubit; final ServicesCubit servicesCubit;
void addJob(final ClientJob job) { void addJob(final ClientJob job) {
final List<ClientJob> newJobsList = []; final jobs = currentJobList;
if (state is JobsStateWithJobs) { if (job.canAddTo(jobs)) {
final JobsStateWithJobs jobsState = state as JobsStateWithJobs; _updateJobsState([
newJobsList.addAll(jobsState.clientJobList); ...jobs,
...[job],
]);
} }
newJobsList.add(job);
getIt<NavigationService>().showSnackBar('jobs.job_added'.tr());
emit(JobsStateWithJobs(newJobsList));
} }
void removeJob(final String id) { void removeJob(final String id) {
@ -39,37 +38,18 @@ class JobsCubit extends Cubit<JobsState> {
emit(newState); emit(newState);
} }
void createOrRemoveServiceToggleJob(final ToggleJob job) { List<ClientJob> get currentJobList {
final List<ClientJob> newJobsList = <ClientJob>[]; final List<ClientJob> jobs = <ClientJob>[];
if (state is JobsStateWithJobs) { if (state is JobsStateWithJobs) {
newJobsList.addAll((state as JobsStateWithJobs).clientJobList); jobs.addAll((state as JobsStateWithJobs).clientJobList);
}
final bool needToRemoveJob = newJobsList
.any((final el) => el is ServiceToggleJob && el.id == job.id);
if (needToRemoveJob) {
final ClientJob removingJob = newJobsList.firstWhere(
(final el) => el is ServiceToggleJob && el.id == job.id,
);
removeJob(removingJob.id);
} else {
newJobsList.add(job);
getIt<NavigationService>().showSnackBar('jobs.job_added'.tr());
emit(JobsStateWithJobs(newJobsList));
} }
return jobs;
} }
void createShhJobIfNotExist(final CreateSSHKeyJob job) { void _updateJobsState(final List<ClientJob> newJobs) {
final List<ClientJob> newJobsList = <ClientJob>[]; getIt<NavigationService>().showSnackBar('jobs.job_added'.tr());
if (state is JobsStateWithJobs) { emit(JobsStateWithJobs(newJobs));
newJobsList.addAll((state as JobsStateWithJobs).clientJobList);
}
final bool isExistInJobList =
newJobsList.any((final el) => el is CreateSSHKeyJob);
if (!isExistInJobList) {
newJobsList.add(job);
getIt<NavigationService>().showSnackBar('jobs.job_added'.tr());
emit(JobsStateWithJobs(newJobsList));
}
} }
Future<void> rebootServer() async { Future<void> rebootServer() async {

49
lib/logic/models/job.dart
View File

@ -17,6 +17,7 @@ abstract class ClientJob extends Equatable {
final String title; final String title;
final String id; final String id;
bool canAddTo(final List<ClientJob> jobs) => true;
void execute(final JobsCubit cubit); void execute(final JobsCubit cubit);
@override @override
@ -25,10 +26,14 @@ abstract class ClientJob extends Equatable {
class RebuildServerJob extends ClientJob { class RebuildServerJob extends ClientJob {
RebuildServerJob({ RebuildServerJob({
required final super.title, required super.title,
final super.id, super.id,
}); });
@override
bool canAddTo(final List<ClientJob> jobs) =>
!jobs.any((final job) => job is RebuildServerJob);
inex marked this conversation as resolved Outdated
inex commented 2022-10-08 11:03:11 +03:00
Outdated
Review

These validations look awful. We have equatable in our project for that.

https://pub.dev/packages/equatable

This library is used in all of our cubits state to compare if they are the same. Should we use it here too?

These validations look awful. We have `equatable` in our project for that. https://pub.dev/packages/equatable This library is used in all of our cubits state to compare if they are the same. Should we use it here too?
NaiJi commented 2022-10-08 17:50:49 +03:00
Outdated
Review

We can't use equatable for it, because it bears different semantics. Even its documentation page says

Equatable overrides == and hashCode

What canAddTo does is not simple checking for equality, if you read these checks again you will see they have different meaning, none of them is checking for absolute equality of job instances, at least because jobs have unique ids.

  • RebuildServerJob checks if there is already any RebuildServerJob instance in the list
  • DeleteUserJob checks if there is already a DeleteUserJob instance with the same login in the list
  • ServiceToggleJob checks if there is already a ServiceToggleJob instance with the same service id in the list
  • DeleteSSHKeyJob checks if there is already a DeleteSSHKeyJob instance with the same both user login and public key at the same time.

The issue is the looks of the solution and I actually don't know how to avoid this. I mean, previously these checks were in JobsCubit, now they are in the jobs themselves, the same approach.

We can't use equatable for it, because it bears different semantics. Even its documentation page says ``` Equatable overrides == and hashCode ``` What `canAddTo` does is not simple checking for equality, if you read these checks again you will see they have different meaning, none of them is checking for absolute equality of job instances, at least because jobs have unique ids. - RebuildServerJob checks if there is already **any** RebuildServerJob instance in the list - DeleteUserJob checks if there is already a DeleteUserJob instance with the same login in the list - ServiceToggleJob checks if there is already a ServiceToggleJob instance with the same service id in the list - DeleteSSHKeyJob checks if there is already a DeleteSSHKeyJob instance with the same both user login and public key at the same time. The issue is the looks of the solution and I actually don't know how to avoid this. I mean, previously these checks were in JobsCubit, now they are in the jobs themselves, the same approach.
inex commented 2022-10-08 18:52:21 +03:00
Outdated
Review

Equatable covers exactly same logic you see using in these checks. Just drop to props fields you want to check, and that's it.

Equatable covers exactly same logic you see using in these checks. Just drop to `props` fields you want to check, and that's it.
inex commented 2022-10-08 18:55:20 +03:00
Outdated
Review

Ah, well, I see that during check you do not construct a Job object, and you nave nothing to compare yet.

Nope, just pass this

~~Ah, well, I see that during check you do not construct a Job object, and you nave nothing to compare yet.~~ Nope, just pass `this`
@override @override
void execute(final JobsCubit cubit) async { void execute(final JobsCubit cubit) async {
await cubit.upgradeServer(); await cubit.upgradeServer();
@ -74,6 +79,11 @@ class DeleteUserJob extends ClientJob {
final User user; final User user;
@override
bool canAddTo(final List<ClientJob> jobs) => !jobs.any(
(final job) => job is DeleteUserJob && job.user.login == user.login,
);
@override @override
void execute(final JobsCubit cubit) async { void execute(final JobsCubit cubit) async {
await cubit.usersCubit.deleteUser(user); await cubit.usersCubit.deleteUser(user);
@ -83,33 +93,30 @@ class DeleteUserJob extends ClientJob {
List<Object> get props => [id, title, user]; List<Object> get props => [id, title, user];
} }
abstract class ToggleJob extends ClientJob { class ServiceToggleJob extends ClientJob {
ToggleJob({
required final this.service,
required final super.title,
});
final Service service;
@override
List<Object> get props => [...super.props, service];
}
class ServiceToggleJob extends ToggleJob {
ServiceToggleJob({ ServiceToggleJob({
required super.service, required this.service,
required this.needToTurnOn, required this.needToTurnOn,
}) : super( }) : super(
title: title:
'${needToTurnOn ? "jobs.service_turn_on".tr() : "jobs.service_turn_off".tr()} ${service.displayName}', '${needToTurnOn ? "jobs.service_turn_on".tr() : "jobs.service_turn_off".tr()} ${service.displayName}',
); );
final bool needToTurnOn;
final Service service;
@override
bool canAddTo(final List<ClientJob> jobs) => !jobs.any(
(final job) => job is ServiceToggleJob && job.service.id == service.id,
);
@override @override
void execute(final JobsCubit cubit) async { void execute(final JobsCubit cubit) async {
await cubit.api.switchService(service.id, needToTurnOn); await cubit.api.switchService(service.id, needToTurnOn);
} }
final bool needToTurnOn; @override
List<Object> get props => [...super.props, service];
} }
class CreateSSHKeyJob extends ClientJob { class CreateSSHKeyJob extends ClientJob {
@ -139,6 +146,14 @@ class DeleteSSHKeyJob extends ClientJob {
final User user; final User user;
final String publicKey; final String publicKey;
@override
bool canAddTo(final List<ClientJob> jobs) => !jobs.any(
(final job) =>
job is DeleteSSHKeyJob &&
job.publicKey == publicKey &&
job.user.login == user.login,
);
@override @override
void execute(final JobsCubit cubit) async { void execute(final JobsCubit cubit) async {
await cubit.usersCubit.deleteSshKey(user, publicKey); await cubit.usersCubit.deleteSshKey(user, publicKey);

32
lib/ui/pages/server_details/server_settings.dart
View File

@ -10,7 +10,6 @@ class _ServerSettings extends StatefulWidget {
class _ServerSettingsState extends State<_ServerSettings> { class _ServerSettingsState extends State<_ServerSettings> {
bool? allowAutoUpgrade; bool? allowAutoUpgrade;
bool? rebootAfterUpgrade; bool? rebootAfterUpgrade;
bool? didSomethingChange;
@override @override
Widget build(final BuildContext context) { Widget build(final BuildContext context) {
@ -25,18 +24,14 @@ class _ServerSettingsState extends State<_ServerSettings> {
rebootAfterUpgrade = serverDetailsState.autoUpgradeSettings.allowReboot; rebootAfterUpgrade = serverDetailsState.autoUpgradeSettings.allowReboot;
} }
didSomethingChange ??= false;
return Column( return Column(
children: [ children: [
SwitchListTile( SwitchListTile(
value: allowAutoUpgrade ?? false, value: allowAutoUpgrade ?? false,
onChanged: (final switched) { onChanged: (final switched) {
if (didSomethingChange == false) { context.read<JobsCubit>().addJob(
context.read<JobsCubit>().addJob( RebuildServerJob(title: 'jobs.upgrade_server'.tr()),
RebuildServerJob(title: 'jobs.upgrade_server'.tr()), );
);
}
context context
.read<ServerDetailsCubit>() .read<ServerDetailsCubit>()
.repository .repository
@ -48,7 +43,6 @@ class _ServerSettingsState extends State<_ServerSettings> {
); );
setState(() { setState(() {
allowAutoUpgrade = switched; allowAutoUpgrade = switched;
didSomethingChange = true;
}); });
}, },
title: Text('server.allow_autoupgrade'.tr()), title: Text('server.allow_autoupgrade'.tr()),
@ -60,11 +54,9 @@ class _ServerSettingsState extends State<_ServerSettings> {
SwitchListTile( SwitchListTile(
value: rebootAfterUpgrade ?? false, value: rebootAfterUpgrade ?? false,
onChanged: (final switched) { onChanged: (final switched) {
if (didSomethingChange == false) { context.read<JobsCubit>().addJob(
context.read<JobsCubit>().addJob( RebuildServerJob(title: 'jobs.upgrade_server'.tr()),
RebuildServerJob(title: 'jobs.upgrade_server'.tr()), );
);
}
context context
.read<ServerDetailsCubit>() .read<ServerDetailsCubit>()
.repository .repository
@ -76,7 +68,6 @@ class _ServerSettingsState extends State<_ServerSettings> {
); );
setState(() { setState(() {
rebootAfterUpgrade = switched; rebootAfterUpgrade = switched;
didSomethingChange = true;
}); });
}, },
title: Text('server.reboot_after_upgrade'.tr()), title: Text('server.reboot_after_upgrade'.tr()),
@ -91,14 +82,9 @@ class _ServerSettingsState extends State<_ServerSettings> {
serverDetailsState.serverTimezone.toString(), serverDetailsState.serverTimezone.toString(),
), ),
onTap: () { onTap: () {
if (didSomethingChange == false) { context.read<JobsCubit>().addJob(
context.read<JobsCubit>().addJob( RebuildServerJob(title: 'jobs.upgrade_server'.tr()),
RebuildServerJob(title: 'jobs.upgrade_server'.tr()), );
);
}
setState(() {
didSomethingChange = true;
});
Navigator.of(context).push( Navigator.of(context).push(
materialRoute( materialRoute(
const SelectTimezone(), const SelectTimezone(),

2
lib/ui/pages/services/service_page.dart
View File

@ -89,7 +89,7 @@ class _ServicePageState extends State<ServicePage> {
ListTile( ListTile(
iconColor: Theme.of(context).colorScheme.onBackground, iconColor: Theme.of(context).colorScheme.onBackground,
onTap: () => { onTap: () => {
context.read<JobsCubit>().createOrRemoveServiceToggleJob( context.read<JobsCubit>().addJob(
ServiceToggleJob( ServiceToggleJob(
service: service, service: service,
needToTurnOn: serviceDisabled, needToTurnOn: serviceDisabled,

3
lib/ui/pages/services/services.dart
View File

@ -157,8 +157,7 @@ class _Card extends StatelessWidget {
return BrandSwitch( return BrandSwitch(
value: isActive, value: isActive,
onChanged: (final value) => onChanged: (final value) => jobsCubit.addJob(
jobsCubit.createOrRemoveServiceToggleJob(
ServiceToggleJob( ServiceToggleJob(
service: service, service: service,
needToTurnOn: value, needToTurnOn: value,