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

NaiJi · 2022-10-07T21:00:03+03:00

NaiJi commented

2022-10-07 21:00:03 +03:00

Jobs now have bool canAddTo(final List<ClientJob> jobs) function to override from abstract ClientJob. It Allows every job type specify how and when exactly they can be added to global jobs list.
JobsCubit now allows clients add new jobs only through plain void addJob(final ClientJob) function to avoid misunderstanding and misusage of cubit's interface.
Abstract ToogleJob is removed as unneeded.
Server Settings page now creates Upgrade Server job properly on changes (issue 131).
Service Toogle jobs now do not multiply.
Remove User jobs now do not multiply.
Remove SSH key jobs now do not multiply.

- Jobs now have `bool canAddTo(final List<ClientJob> jobs)` function to override from abstract ClientJob. It Allows every job type specify how and when exactly they can be added to global jobs list. - JobsCubit now allows clients add new jobs **only** through plain `void addJob(final ClientJob)` function to avoid misunderstanding and misusage of cubit's interface. - Abstract ToogleJob is removed as unneeded. - Server Settings page now creates Upgrade Server job properly on changes (issue [131](https://git.selfprivacy.org/kherel/selfprivacy.org.app/issues/131)). - Service Toogle jobs now do not multiply. - Remove User jobs now do not multiply. - Remove SSH key jobs now do not multiply.

❤️ 1

NaiJi added the

Feature request

label 2022-10-07 21:00:03 +03:00

NaiJi self-assigned this 2022-10-07 21:00:03 +03:00

NaiJi added 2 commits 2022-10-07 21:00:04 +03:00

db2f5c1342 fix(job): Fix server settings toogles not creating upgrade server job

7bad11967a refactor(job): Implement polymorphic predicate for job accessibility

Now every job type can impement canAddTo function to make JobsCubit know whether it can be applied or not

NaiJi requested review from inex 2022-10-07 21:00:11 +03:00

NaiJi referenced this pull request

2022-10-07 21:07:21 +03:00

Server settings toogle elements do not create jobs #131

inex requested changes 2022-10-08 11:04:42 +03:00

inex left a comment

Looks good, but we should evaluate if using equatable for canAddTo checks would make code cleaner.

Looks good, but we should evaluate if using `equatable` for `canAddTo` checks would make code cleaner.

lib/logic/models/job.dart Outdated

													
				@ -31,1 +32,4 @@

				  @override

				  bool canAddTo(final List<ClientJob> jobs) =>

				      super.canAddTo(jobs) && !jobs.any((final job) => job is RebuildServerJob);

inex commented

2022-10-08 11:03:11 +03:00

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

Poster

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

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

~~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`

inex marked this conversation as resolved

NaiJi requested review from inex 2022-10-08 17:51:07 +03:00

NaiJi commented

2022-10-08 17:55:57 +03:00

Poster

A simpler way to say it, canAddTo bears business logic semantics, not technical, so we can't plainly compare instances of Job types for absolute equality

NaiJi force-pushed server-settings from 973c7d3e8a to 0b5f8b6920

2022-10-08 19:20:07 +03:00

Compare

inex approved these changes 2022-10-08 19:21:51 +03:00

NaiJi merged commit b741399ba9 into master

2022-10-08 19:22:46 +03:00

NaiJi deleted branch server-settings

2022-10-08 19:22:46 +03:00

NaiJi referenced this issue from a commit

2022-10-08 19:22:47 +03:00

Merge pull request 'refactor(job): Implement polymorphic behavior on creation for jobs' (#134) from server-settings into master

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: SelfPrivacy/selfprivacy.org.app#134