From 3e332d4ff3d80362f92ce8e029406acf66fb64f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Ca=C5=82ka?= <25438601+rafaucau@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:31:33 +0100 Subject: [PATCH 01/11] fix(optional-policies): resolve `Attempt to read property "is_accepted" on null` warnings --- extend.php | 10 ++++----- src/Repositories/PolicyRepository.php | 29 +++++++++------------------ 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/extend.php b/extend.php index 475b884..8abb8f0 100644 --- a/extend.php +++ b/extend.php @@ -47,12 +47,10 @@ ->add(RegisterMiddleware::class), (new Extend\Model(User::class)) - ->relationship('fofTermsPolicies', function (AbstractModel $user): BelongsToMany { - return $user->belongsToMany(Policy::class, 'fof_terms_policy_user')->withPivot('accepted_at'); - }) - ->relationship('fofTermsPoliciesState', function (AbstractModel $user): BelongsToMany { - return $user->belongsToMany(Policy::class, 'fof_terms_policy_user')->withPivot('is_accepted'); - }), + ->relationship('fofTermsPolicies', fn(AbstractModel $user): BelongsToMany => + $user + ->belongsToMany(Policy::class, 'fof_terms_policy_user') + ->withPivot(['accepted_at', 'is_accepted'])), (new Extend\User()) ->permissionGroups(function ($actor, $groupIds) { diff --git a/src/Repositories/PolicyRepository.php b/src/Repositories/PolicyRepository.php index dbb452f..7fd46c4 100644 --- a/src/Repositories/PolicyRepository.php +++ b/src/Repositories/PolicyRepository.php @@ -12,7 +12,6 @@ namespace FoF\Terms\Repositories; use Carbon\Carbon; -use DateTime; use Flarum\User\User; use FoF\Terms\Events\Created; use FoF\Terms\Events\Deleted; @@ -27,14 +26,11 @@ class PolicyRepository { - protected $policy; - protected $validator; - protected $cache; + protected Policy $policy; + protected PolicyValidator $validator; + protected Repository $cache; - /** - * @var Dispatcher - */ - protected $events; + protected Dispatcher $events; protected $rememberState; @@ -49,7 +45,7 @@ public function __construct(Policy $policy, PolicyValidator $validator, Reposito } /** - * @return Policy[]|Collection + * @return Collection */ public function all(): Collection { @@ -77,8 +73,7 @@ public function state(User $user) { if (!$this->rememberState) { /** - * @var Collection $userPolicies - * + * @var Collection $userPolicies * @phpstan-ignore-next-line */ $userPolicies = $user->fofTermsPolicies->keyBy('id'); @@ -89,17 +84,11 @@ public function state(User $user) $accepted_at = $userPolicies->has($policy->id) ? Carbon::parse($userPolicies->get($policy->id)->pivot->accepted_at) : null; $has_update = !$accepted_at || (($policy->terms_updated_at !== null) && $policy->terms_updated_at->gt($accepted_at)); $optional = $policy->optional; - - /** - * @var bool $is_accepted - * - * @phpstan-ignore-next-line - */ - $is_accepted = $user->fofTermsPoliciesState->keyBy('id')->get($policy->id)->pivot->is_accepted; + /** @var bool $is_accepted */ + $is_accepted = $userPolicies->has($policy->id) ? $userPolicies->get($policy->id)->pivot->is_accepted : false; $this->rememberState[$policy->id] = [ - // Same format as Flarum is using for the serializer responses - 'accepted_at' => $accepted_at ? $accepted_at->format(DateTime::RFC3339) : null, + 'accepted_at' => $accepted_at ? $accepted_at->toRfc3339String() : null, 'has_update' => $has_update, 'must_accept' => $has_update && !$user->can('postponeAccept', $policy) && !$optional, 'is_accepted' => $is_accepted, From 5539e46b2c63b2e28fb4754dd249b4a84b142879 Mon Sep 17 00:00:00 2001 From: rafaucau Date: Wed, 4 Dec 2024 09:31:49 +0000 Subject: [PATCH 02/11] Apply fixes from StyleCI --- extend.php | 3 +-- src/Repositories/PolicyRepository.php | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/extend.php b/extend.php index 8abb8f0..2a461bf 100644 --- a/extend.php +++ b/extend.php @@ -47,8 +47,7 @@ ->add(RegisterMiddleware::class), (new Extend\Model(User::class)) - ->relationship('fofTermsPolicies', fn(AbstractModel $user): BelongsToMany => - $user + ->relationship('fofTermsPolicies', fn (AbstractModel $user): BelongsToMany => $user ->belongsToMany(Policy::class, 'fof_terms_policy_user') ->withPivot(['accepted_at', 'is_accepted'])), diff --git a/src/Repositories/PolicyRepository.php b/src/Repositories/PolicyRepository.php index 7fd46c4..eee59ef 100644 --- a/src/Repositories/PolicyRepository.php +++ b/src/Repositories/PolicyRepository.php @@ -74,6 +74,7 @@ public function state(User $user) if (!$this->rememberState) { /** * @var Collection $userPolicies + * * @phpstan-ignore-next-line */ $userPolicies = $user->fofTermsPolicies->keyBy('id'); From 1ffbb18ba73a201c21027f43409c746d6070f9b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Ca=C5=82ka?= <25438601+rafaucau@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:40:42 +0100 Subject: [PATCH 03/11] fix(optional-policies): calm down some PHPStan errors --- src/Repositories/PolicyRepository.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Repositories/PolicyRepository.php b/src/Repositories/PolicyRepository.php index eee59ef..11bf239 100644 --- a/src/Repositories/PolicyRepository.php +++ b/src/Repositories/PolicyRepository.php @@ -59,13 +59,9 @@ public function clearCache() $this->cache->forget(self::CACHE_KEY); } - /** - * @param string $id - * - * @return Policy - */ public function findOrFail(string $id): Policy { + /** @phpstan-ignore-next-line Method FoF\Terms\Repositories\PolicyRepository::findOrFail() should return FoF\Terms\Policy but returns Illuminate\Database\Eloquent\Model. */ return $this->policy->newQuery()->findOrFail($id); } @@ -82,10 +78,14 @@ public function state(User $user) $this->rememberState = []; foreach ($this->all() as $policy) { + /** @phpstan-ignore-next-line Access to an undefined property FoF\Terms\Policy::$pivot */ $accepted_at = $userPolicies->has($policy->id) ? Carbon::parse($userPolicies->get($policy->id)->pivot->accepted_at) : null; $has_update = !$accepted_at || (($policy->terms_updated_at !== null) && $policy->terms_updated_at->gt($accepted_at)); $optional = $policy->optional; - /** @var bool $is_accepted */ + /** + * @var bool $is_accepted + * @phpstan-ignore-next-line Access to an undefined property FoF\Terms\Policy::$pivot + */ $is_accepted = $userPolicies->has($policy->id) ? $userPolicies->get($policy->id)->pivot->is_accepted : false; $this->rememberState[$policy->id] = [ From b359da6b04b4d882f871b5eb5e425f1c429dc392 Mon Sep 17 00:00:00 2001 From: rafaucau Date: Wed, 4 Dec 2024 09:40:53 +0000 Subject: [PATCH 04/11] Apply fixes from StyleCI --- src/Repositories/PolicyRepository.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Repositories/PolicyRepository.php b/src/Repositories/PolicyRepository.php index 11bf239..8bb673e 100644 --- a/src/Repositories/PolicyRepository.php +++ b/src/Repositories/PolicyRepository.php @@ -84,6 +84,7 @@ public function state(User $user) $optional = $policy->optional; /** * @var bool $is_accepted + * * @phpstan-ignore-next-line Access to an undefined property FoF\Terms\Policy::$pivot */ $is_accepted = $userPolicies->has($policy->id) ? $userPolicies->get($policy->id)->pivot->is_accepted : false; From dbbc0a0da04332d95844e691461a5fbc16de5450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Ca=C5=82ka?= <25438601+rafaucau@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:54:54 +0100 Subject: [PATCH 05/11] fix(optional-policies): make user settings label translatable --- js/src/forum/components/addManagePoliciesOption.js | 4 ++-- resources/locale/en.yml | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/js/src/forum/components/addManagePoliciesOption.js b/js/src/forum/components/addManagePoliciesOption.js index 348547e..bfafd9d 100644 --- a/js/src/forum/components/addManagePoliciesOption.js +++ b/js/src/forum/components/addManagePoliciesOption.js @@ -25,11 +25,11 @@ export default function () { items.add( 'policies', -
+
{optionalPolicies.map((policy) => { const { is_accepted } = policyState[policy.id()]; return ( -
, - -200 + -99 ); }); } From 193cedbd1111eb2244cc53e08dd6ac8ac7c5272b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Ca=C5=82ka?= <25438601+rafaucau@users.noreply.github.com> Date: Wed, 4 Dec 2024 11:00:28 +0100 Subject: [PATCH 07/11] style(optional-policies): remove unnecessary space --- src/Repositories/PolicyRepository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Repositories/PolicyRepository.php b/src/Repositories/PolicyRepository.php index 8bb673e..224beb9 100644 --- a/src/Repositories/PolicyRepository.php +++ b/src/Repositories/PolicyRepository.php @@ -61,7 +61,7 @@ public function clearCache() public function findOrFail(string $id): Policy { - /** @phpstan-ignore-next-line Method FoF\Terms\Repositories\PolicyRepository::findOrFail() should return FoF\Terms\Policy but returns Illuminate\Database\Eloquent\Model. */ + /** @phpstan-ignore-next-line Method FoF\Terms\Repositories\PolicyRepository::findOrFail() should return FoF\Terms\Policy but returns Illuminate\Database\Eloquent\Model. */ return $this->policy->newQuery()->findOrFail($id); } From cbe6437356d262eccce80aa85072c2cb4c52f832 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Ca=C5=82ka?= <25438601+rafaucau@users.noreply.github.com> Date: Thu, 5 Dec 2024 10:09:55 +0100 Subject: [PATCH 08/11] refactor(optional-policies): convert UpdateAlert and Policy model to TS --- js/src/@types/Model/User.d.ts | 17 +++++ js/src/common/models/{Policy.js => Policy.ts} | 12 ++-- js/src/forum/components/UpdateAlert.js | 68 ------------------- js/src/forum/components/UpdateAlert.tsx | 65 ++++++++++++++++++ 4 files changed, 88 insertions(+), 74 deletions(-) create mode 100644 js/src/@types/Model/User.d.ts rename js/src/common/models/{Policy.js => Policy.ts} (54%) delete mode 100644 js/src/forum/components/UpdateAlert.js create mode 100644 js/src/forum/components/UpdateAlert.tsx diff --git a/js/src/@types/Model/User.d.ts b/js/src/@types/Model/User.d.ts new file mode 100644 index 0000000..a9b11cc --- /dev/null +++ b/js/src/@types/Model/User.d.ts @@ -0,0 +1,17 @@ +export * from 'flarum/common/models/User'; + +declare module 'flarum/common/models/User' { + export default interface User { + fofTermsPoliciesHasUpdate(): boolean; + fofTermsPoliciesMustAccept(): boolean; + fofTermsPoliciesState(): Record< + number, + { + accepted_at: string; + has_update: boolean; + is_accepted: boolean; + must_accept: boolean; + } + >; + } +} diff --git a/js/src/common/models/Policy.js b/js/src/common/models/Policy.ts similarity index 54% rename from js/src/common/models/Policy.js rename to js/src/common/models/Policy.ts index 88a54de..32f8943 100644 --- a/js/src/common/models/Policy.js +++ b/js/src/common/models/Policy.ts @@ -2,12 +2,12 @@ import Model from 'flarum/common/Model'; import computed from 'flarum/common/utils/computed'; export default class Policy extends Model { - sort = Model.attribute('sort'); - name = Model.attribute('name'); - url = Model.attribute('url'); - update_message = Model.attribute('update_message'); - terms_updated_at = Model.attribute('terms_updated_at'); - optional = Model.attribute('optional'); + sort = Model.attribute('sort'); + name = Model.attribute('name'); + url = Model.attribute('url'); + update_message = Model.attribute('update_message'); + terms_updated_at = Model.attribute('terms_updated_at'); + optional = Model.attribute('optional'); additional_info = Model.attribute('additional_info'); form_key = computed('id', (id) => 'fof_terms_policy_' + id); diff --git a/js/src/forum/components/UpdateAlert.js b/js/src/forum/components/UpdateAlert.js deleted file mode 100644 index b241c06..0000000 --- a/js/src/forum/components/UpdateAlert.js +++ /dev/null @@ -1,68 +0,0 @@ -import app from 'flarum/forum/app'; -import Button from 'flarum/common/components/Button'; -import listItems from 'flarum/common/helpers/listItems'; -import AcceptPoliciesModal from './AcceptPoliciesModal'; - -/* global m */ - -let temporarilyHidden = false; - -/** - * Renders similarly to Flarum's Alert, but with an additional .container inside - */ -export default class UpdateAlert { - shouldShowAlert() { - if (temporarilyHidden) { - return false; - } - - const user = app.session.user; - - return user && user.fofTermsPoliciesHasUpdate(); - } - - view() { - if (!this.shouldShowAlert()) { - return m('div'); - } - - const controls = [ - Button.component( - { - className: 'Button Button--link', - onclick: () => { - app.modal.show(AcceptPoliciesModal); - }, - }, - app.translator.trans('fof-terms.forum.update-alert.review') - ), - ]; - - const dismissControl = []; - - if (!app.session.user.fofTermsPoliciesMustAccept()) { - dismissControl.push( - Button.component({ - icon: 'fas fa-times', - className: 'Button Button--link Button--icon Alert-dismiss', - onclick: () => { - temporarilyHidden = true; - }, - }) - ); - } - - return m( - '.Alert.Alert-info', - m('.container', [ - m( - 'span.Alert-body', - app.session.user.fofTermsPoliciesMustAccept() - ? app.translator.trans('fof-terms.forum.update-alert.must-accept-message') - : app.translator.trans('fof-terms.forum.update-alert.can-accept-message') - ), - m('ul.Alert-controls', listItems(controls.concat(dismissControl))), - ]) - ); - } -} diff --git a/js/src/forum/components/UpdateAlert.tsx b/js/src/forum/components/UpdateAlert.tsx new file mode 100644 index 0000000..3051749 --- /dev/null +++ b/js/src/forum/components/UpdateAlert.tsx @@ -0,0 +1,65 @@ +import app from 'flarum/forum/app'; +import Button from 'flarum/common/components/Button'; +import listItems from 'flarum/common/helpers/listItems'; +import AcceptPoliciesModal from './AcceptPoliciesModal'; + +let temporarilyHidden = false; + +/** + * Renders similarly to Flarum's Alert, but with an additional .container inside + */ +export default class UpdateAlert { + shouldShowAlert() { + if (temporarilyHidden) { + return false; + } + + const user = app.session.user; + + return user && user.fofTermsPoliciesHasUpdate(); + } + + view() { + if (!this.shouldShowAlert() || !app.session.user) { + return null; + } + + const controls = [ + , + ]; + + const dismissControl = []; + + if (!app.session.user.fofTermsPoliciesMustAccept()) { + dismissControl.push( + ); } - return policies.map((policy) => - m('div', [ - m('h2', policy.name()), - app.forum.attribute('fof-terms.hide-updated-at') - ? null - : m( - 'p', - policy.terms_updated_at() - ? app.translator.trans('fof-terms.forum.accept-modal.updated-at', { - date: dayjs(policy.terms_updated_at()).format(app.forum.attribute('fof-terms.date-format')), - }) - : app.translator.trans('fof-terms.forum.accept-modal.updated-recently') - ), - policy.update_message() ? m('p', policy.update_message()) : null, - m( - '.Form-group', - m( - '.FoF-Terms-Check.FoF-Terms-Check--login', - m('label.checkbox', [ - m('input', { - type: 'checkbox', - checked: this[policy.form_key()], - onchange: () => { + return policies.map((policy) => ( +
+

{policy.name()}

+ {app.forum.attribute('fof-terms.hide-updated-at') ? null : ( +

+ {policy.terms_updated_at() + ? app.translator.trans('fof-terms.forum.accept-modal.updated-at', { + date: dayjs(policy.terms_updated_at()).format(app.forum.attribute('fof-terms.date-format')), + }) + : app.translator.trans('fof-terms.forum.accept-modal.updated-recently')} +

+ )} + {policy.update_message() ?

{policy.update_message()}

: null} +
+
+ +
+
+ +
+ )); } } diff --git a/resources/locale/en.yml b/resources/locale/en.yml index 49d35d5..d878bad 100644 --- a/resources/locale/en.yml +++ b/resources/locale/en.yml @@ -71,7 +71,7 @@ fof-terms: update-alert: must-accept-message: We've temporarily restricted access to your account while you check out the new terms. - can-accept-message: We recently updated the terms. You can accept them to continue using this website. + can-accept-message: We recently updated the terms. You must accept them to continue using this website. review: Click here to review and accept the new terms close: Close From b39bcb3a1f61a38b78a8623cf25cd466e9f06770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Ca=C5=82ka?= <25438601+rafaucau@users.noreply.github.com> Date: Thu, 5 Dec 2024 12:52:40 +0100 Subject: [PATCH 11/11] feat(optional-policies): add a separate alert message for optional policy updates --- .../forum/components/AcceptPoliciesModal.js | 4 +++- js/src/forum/components/UpdateAlert.tsx | 21 +++++++++++++------ resources/locale/en.yml | 1 + 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/js/src/forum/components/AcceptPoliciesModal.js b/js/src/forum/components/AcceptPoliciesModal.js index e9e409b..2b59660 100644 --- a/js/src/forum/components/AcceptPoliciesModal.js +++ b/js/src/forum/components/AcceptPoliciesModal.js @@ -10,7 +10,9 @@ export default class AcceptPoliciesModal extends Modal { super.oninit(vnode); app.store.all('fof-terms-policies').forEach((policy) => { - this[policy.form_key()] = false; + const state = app.session.user.fofTermsPoliciesState()[policy.id()]; + // For optional policies, maintain current acceptance status + this[policy.form_key()] = policy.optional() ? state?.is_accepted || false : false; }); } diff --git a/js/src/forum/components/UpdateAlert.tsx b/js/src/forum/components/UpdateAlert.tsx index 8f938a9..1a2e648 100644 --- a/js/src/forum/components/UpdateAlert.tsx +++ b/js/src/forum/components/UpdateAlert.tsx @@ -14,13 +14,20 @@ export default class UpdateAlert { return false; } - const user = app.session.user; + const { user } = app.session; return user && user.fofTermsPoliciesHasUpdate(); } + hasOnlyOptionalUpdates() { + const { user } = app.session; + return user && !user.fofTermsPoliciesMustAccept() && user.fofTermsPoliciesHasUpdate(); + } + view() { - if (!this.shouldShowAlert() || !app.session.user) { + const { user } = app.session; + + if (!this.shouldShowAlert() || !user) { return null; } @@ -37,7 +44,7 @@ export default class UpdateAlert { const dismissControl = []; - if (!app.session.user.fofTermsPoliciesMustAccept()) { + if (!user.fofTermsPoliciesMustAccept()) { dismissControl.push(