Pause ALL my notifications #910

dzonidoo · 2024-05-22T17:58:19Z

https://sofab.atlassian.net/browse/CPCN-186

Checklist

This pull request is not adding new forms that use redux
This pull request is adding missing TypeScript types to modified code segments where it's easy to do so with confidence
This pull request is replacing lodash.get with optional chaining for modified code segments

tomaskikutis · 2024-05-23T10:12:25Z

@thecalcc can you review this?

thecalcc · 2024-05-23T11:17:45Z

assets/components/NotificationList.tsx


-class NotificationList extends React.Component<any, any> {


If it's quick, can you type the props as well?

thecalcc · 2024-05-23T11:19:00Z

assets/components/NotificationList.tsx

+                    )}
+                    ref={(elem: any) => this.elem = elem}
+                    title={gettext('Notifications')}>
+                    <h3 className="a11y-only">Notification bell</h3>


add gettext here, we shouldn't care if it's hidden

thecalcc · 2024-05-23T11:19:29Z

assets/notifications/actions.ts


 export const UPDATE_NOTIFICATION_COUNT = 'UPDATE_NOTIFICATION_COUNT';
 export function updateNotificationCount(count: any) {
    return {type: UPDATE_NOTIFICATION_COUNT, count};
 }

 export const INIT_DATA = 'INIT_DATA';
-export function initData(data: any) {
-    return {type: INIT_DATA, data};
+export function initData(notificationData: any, profileData: any) {


can you type props here?

thecalcc · 2024-05-23T11:21:12Z

assets/user-profile/components/PauseNotificationModal.tsx

+        if (this.formRef.current == null) {
+            throw new Error('ref missing');
+        }
+


is this meant to stop the function execution? What does throwing an error serve us here?

There are a few reasons for this. One is to stop the execution in case it is null and another is to tell compiler that if execution is not stopped, it should be treated as non-null.

We could also achieve this using if/else or early return. But - it is preferrable to throw errors when we're guarding against invalid application states.

thecalcc · 2024-05-23T11:21:47Z

assets/user-profile/components/PauseNotificationModal.tsx

+        const hasErrors = [...inputs].some(input => {
+            return input.checkValidity() !== true;
+        });


what's the reason here to spread inputs?

querySelectorAll returns a collection which is not an array and doesn't have array methods. @dzonidoo use Array.from(inputs) instead.

thecalcc · 2024-05-23T11:23:28Z

assets/user-profile/components/PauseNotificationModal.tsx

+            const newMaxDate = new Date(state);
+            newMaxDate.setDate(newMaxDate.getDate() + 1);


you can do this in one line, but if you think readability is better this way, let's keep it.

Add 1 to a constant that explains what its purpose is, we can't have magic numbers

when 0 and 1 are used, most of the times it's obvious what's being done thus 0/1 are not considered magic numbers.

thecalcc · 2024-05-23T11:24:08Z

assets/user-profile/components/PauseNotificationModal.tsx

+    disabledMaxOption(state: string | undefined) {
+        if (state != '' && state != undefined) {
+            const newMaxDate = new Date(state);
+            newMaxDate.setDate(newMaxDate.getDate() - 1);


same goes for here, I bet 1 is some kind of offset, I understand it but others would need to know also

thecalcc · 2024-05-23T11:25:01Z

assets/user-profile/components/PauseNotificationModal.tsx

+                                    type="date"
+                                    name="date-from"
+                                    className="form-control"
+                                    onChange={(event) => this.updateDate(event, 'pauseFrom')}


looks like we're expecting a return from this function which isn't the case, can you add curly braces to it?

tomaskikutis

@thecalcc you shouldn't approve a PR until your comments are addressed/answered.

I'll also block the merging so after you finish the review, I'll take a look too.

thecalcc · 2024-05-23T11:45:55Z

@thecalcc you shouldn't approve a PR until your comments are addressed/answered.

I'll also block the merging so after you finish the review, I'll take a look too.

I clicked approve by mistake, my bad

thecalcc

Would component prop types take too long to be properly typed?

dzonidoo · 2024-05-27T09:40:58Z

Actually yes...

thecalcc · 2024-05-27T10:40:26Z

Actually yes...

@dzonidoo ok got it. What about not adding new redux forms, is it possible that we don't use redux?

dzonidoo · 2024-05-29T09:34:09Z

@thecalcc its not.. we need it

tomaskikutis

Thanks for taking a look @thecalcc, I'll take over from here. I wish you did a more in-depth job, I still have lots of comments after your review.

@dzonidoo there's a bug I found not related to this PR - do apply the following patch to fix it. (it was crashing when notification schedule isn't set)

assets/components/NotificationList.tsx

assets/user-profile/actions.ts

assets/components/NotificationList.tsx

assets/user-profile/components/PauseNotificationModal.tsx

tomaskikutis · 2024-05-31T11:32:13Z

assets/user-profile/components/PauseNotificationModal.tsx

+        if (this.formRef.current == null) {
+            throw new Error('ref missing');
+        }
+


There are a few reasons for this. One is to stop the execution in case it is null and another is to tell compiler that if execution is not stopped, it should be treated as non-null.

We could also achieve this using if/else or early return. But - it is preferrable to throw errors when we're guarding against invalid application states.

tomaskikutis · 2024-05-31T11:34:07Z

assets/user-profile/components/PauseNotificationModal.tsx

+        const hasErrors = [...inputs].some(input => {
+            return input.checkValidity() !== true;
+        });


querySelectorAll returns a collection which is not an array and doesn't have array methods. @dzonidoo use Array.from(inputs) instead.

tomaskikutis · 2024-05-31T11:36:35Z

assets/user-profile/components/PauseNotificationModal.tsx

+            const newMaxDate = new Date(state);
+            newMaxDate.setDate(newMaxDate.getDate() + 1);


when 0 and 1 are used, most of the times it's obvious what's being done thus 0/1 are not considered magic numbers.

tomaskikutis · 2024-05-31T11:43:54Z

What about not adding new redux forms, is it possible that we don't use redux?

where did you see Redux being used for forms @thecalcc ? If data that you gather from a form is being sent to a redux action, it doesn't make it a redux based form. Those are when validation and form data while typing live inside redux store.

assets/helpers/notification.tsx

assets/components/NotificationList.tsx

assets/user-profile/components/PauseNotificationModal.tsx

assets/notifications/components/NotificationsApp.tsx

assets/search/components/TopicEditor.tsx

assets/user-profile/components/EditNotificationScheduleModal.tsx

assets/user-profile/components/profile/UserProfile.tsx

assets/user-profile/components/PauseNotificationModal.tsx

tomaskikutis

also check one other comment I left unresolved

tomaskikutis · 2024-06-16T15:21:10Z

assets/components/NotificationList.tsx

@@ -70,9 +85,11 @@ class NotificationList extends React.Component<any, any> {
    }

    render() {
+        const notificationArePaused: boolean = new Date(this.props.fullUser.notification_schedule?.pauseFrom ?? '') < new Date();


You should't be creating a date from an empty string. If it's not defined, simply set notificationArePaused to false and compare dates only if it's defined.

pasue notification

d9640be

dzonidoo requested review from tomaskikutis and petrjasek May 22, 2024 17:58

tomaskikutis requested review from thecalcc and removed request for petrjasek May 23, 2024 10:01

thecalcc approved these changes May 23, 2024

View reviewed changes

tomaskikutis requested changes May 23, 2024

View reviewed changes

changes after review

6bbd6ea

dzonidoo requested a review from thecalcc May 23, 2024 13:19

Merge branch 'develop' into pause-notification

ec61a4b

petrjasek added this to the v2.8 milestone May 24, 2024

thecalcc reviewed May 27, 2024

View reviewed changes

tomaskikutis reviewed May 31, 2024

View reviewed changes

dzonidoo added 2 commits June 4, 2024 11:18

Merge branch 'develop' into pause-notification

a56070a

change after review part II

2d41bc7

dzonidoo requested a review from tomaskikutis June 4, 2024 17:01

dzonidoo added 2 commits June 6, 2024 13:18

update user in user-profile section fix

352bf48

update fullUser in profile-section fix part II

69ff164

tomaskikutis reviewed Jun 7, 2024

View reviewed changes

dzonidoo added 2 commits June 11, 2024 12:57

changes after review part III

319acb8

Merge branch 'develop' into pause-notification

32afcbe

dzonidoo requested a review from tomaskikutis June 12, 2024 12:42

dzonidoo added 2 commits June 13, 2024 10:58

fix lint

653b3ff

fix lint

6766886

tomaskikutis reviewed Jun 16, 2024

View reviewed changes

dzonidoo added 2 commits June 17, 2024 11:49

changes after review part IV

643db3a

fix lint and type of updateUserNotificationSchedule functnion

ee0c552

dzonidoo requested a review from tomaskikutis June 17, 2024 10:54

tomaskikutis approved these changes Jun 17, 2024

View reviewed changes

dzonidoo merged commit 6072e7a into superdesk:develop Jun 17, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pause ALL my notifications #910

Pause ALL my notifications #910

dzonidoo commented May 22, 2024

tomaskikutis commented May 23, 2024

thecalcc May 23, 2024

thecalcc May 23, 2024

thecalcc May 23, 2024

thecalcc May 23, 2024

tomaskikutis May 31, 2024

thecalcc May 23, 2024

tomaskikutis May 31, 2024

thecalcc May 23, 2024

tomaskikutis May 31, 2024

thecalcc May 23, 2024

thecalcc May 23, 2024

tomaskikutis left a comment

thecalcc commented May 23, 2024

thecalcc left a comment

dzonidoo commented May 27, 2024

thecalcc commented May 27, 2024 •

edited

Loading

dzonidoo commented May 29, 2024

tomaskikutis left a comment

tomaskikutis May 31, 2024

tomaskikutis May 31, 2024

tomaskikutis May 31, 2024

tomaskikutis commented May 31, 2024 •

edited

Loading

tomaskikutis left a comment

tomaskikutis Jun 16, 2024

		const newMaxDate = new Date(state);
		newMaxDate.setDate(newMaxDate.getDate() + 1);

Pause ALL my notifications #910

Pause ALL my notifications #910

Conversation

dzonidoo commented May 22, 2024

Checklist

tomaskikutis commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaskikutis left a comment

Choose a reason for hiding this comment

thecalcc commented May 23, 2024

thecalcc left a comment

Choose a reason for hiding this comment

dzonidoo commented May 27, 2024

thecalcc commented May 27, 2024 • edited Loading

dzonidoo commented May 29, 2024

tomaskikutis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaskikutis commented May 31, 2024 • edited Loading

tomaskikutis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecalcc commented May 27, 2024 •

edited

Loading

tomaskikutis commented May 31, 2024 •

edited

Loading