Implementation/59721 add setedit reminder buttondialog to workpackage page #17341

jjabari-op · 2024-12-03T16:06:47Z

Ticket

https://community.openproject.org/work_packages/59721

…ic spec coverage

…tondialog-to-workpackage-page

...tures/work-packages/components/wp-buttons/wp-reminder-button/wp-reminder-button.component.ts

frontend/src/app/features/work-packages/components/wp-reminder-modal/wp-reminder.modal.ts

akabiru

Quite the feat, nicely done! 👏🏾 I've noted a few general issues

Missing unit test coverage for model, contract and API changes - annotated in the code
Missing API specification & tests
Several eslint and rubocop issues

UI Wise

Missing modal cancel button as per the design
- Separately, it would be good to check with UX/Product if we can adopt "Remove reminder" instead of "Delete reminder" and "Schedule" instead of "Save" - from previous mention people seemed in favour of the idea
On hover of the "Set reminder icon" no label is displayed
The reminders clock icon is off-centered to the left

Content loader skeleton does not match the reminders UI
Duplicate form error messages

Unhandled 404 when reminder edit modal is open, and the reminder notification is created whilst it's still open (you can test this by creating a reminder in the next minute, open the edit modal and wait for the reminder notification to be created then attempt to edit it)

icon is smaller than counter

app/contracts/reminders/base_contract.rb

akabiru · 2024-12-06T10:18:15Z

app/controllers/work_packages/reminders_controller.rb

+  # At the form level, we split the date and time into two form fields.
+  # In order to be a bit more informative of which field is causing
+  # the remind_at attribute to be in the past/invalid, we need to
+  # remap the error attribute to the appropriate field.
+  def prepare_errors_from_result(service_result)
+    # We set the reminder here for "create" case
+    # as the record comes from the service.
+    @reminder = service_result.result
+    @errors = service_result.errors
+
+    case @errors.find { |error| error.attribute == :remind_at }&.type
+    when :blank
+      handle_blank_error
+    when :datetime_must_be_in_future
+      handle_future_error
+    end
+
+    @errors.delete(:remind_at)
+  end


🟠 I think this method and related calls would do well as an extracted simple PORO that can be unit tested- also reduce the perceived complexity on the controller

https://community.openproject.org/wp/60127

akabiru · 2024-12-06T10:20:10Z

app/models/reminder.rb

+  # Currently, reminders are personal, meaning
+  # they are only visible to the user who created them.
+  def self.visible(user)
+    where(creator: user)
+  end
+
+  def self.upcoming_and_visible_to(user)
+    visible_reminders = visible(user)
+    reminder_notifications_for_reminders = ReminderNotification.where(reminder: visible_reminders)
+
+    visible_reminders
+      .where(completed_at: nil)
+      .where.not(id: reminder_notifications_for_reminders.select(:reminder_id))
+  end
+


Nice one covering these- 🟠 it would be worth add the corresponding model unit specs in https://github.com/opf/openproject/blob/dev/spec/models/reminder_spec.rb

🟠 Once the tests are added. I think the method to exclude reminders that have associated reminder notifications can be simplified via where.missing

Performance seems ~~about the same~~ faster - as the later results in no sub-queries and reads easier IMO

Suggested change

# Currently, reminders are personal, meaning

# they are only visible to the user who created them.

def self.visible(user)

where(creator: user)

end

def self.upcoming_and_visible_to(user)

visible_reminders = visible(user)

reminder_notifications_for_reminders = ReminderNotification.where(reminder: visible_reminders)

visible_reminders

.where(completed_at: nil)

.where.not(id: reminder_notifications_for_reminders.select(:reminder_id))

end

# Currently, reminders are personal, meaning

# they are only visible to the user who created them.

def self.visible(user)

where(creator: user)

end

def self.upcoming_and_visible_to(user)

visible(user)

.where(completed_at: nil)

.where.missing(:reminder_notifications)

end

akabiru · 2024-12-06T10:37:36Z

frontend/src/app/features/work-packages/components/wp-reminder-modal/wp-reminder.modal.html

+        <svg:rect height="10" rx="2" ry="2" width="110" x="0" y="0"/>
+        <svg:rect height="10" rx="2" ry="2" width="25" x="120" y="0"/>
+        <svg:rect height="10" rx="2" ry="2" width="25" x="155" y="0"/>
+        <svg:rect height="25%" rx="2" ry="2" width="180" x="0" y="16"/>


🟠 I noticed this content loader skeleton comes from the shares modal- could we ask design for one that matches the reminders modal?

akabiru · 2024-12-06T10:39:27Z

...tures/work-packages/components/wp-buttons/wp-reminder-button/wp-reminder-button.component.ts

+        map((collection:CollectionResource) => { return collection.total; }),
+      );
+  }
+}


🧹 Several eslint issues that need fixing

akabiru · 2024-12-06T11:05:08Z

...tures/work-packages/components/wp-buttons/wp-reminder-button/wp-reminder-button.component.ts

+    this.reminderCount$ = merge(
+      this
+        .actions$
+        .ofType(reminderModalUpdated)
+        .pipe(
+          map((action) => action.workPackageId),
+          filter((id) => id === this.workPackage.id?.toString()),
+          startWith(null),
+          switchMap(() => this.countReminders()),
+        ),
+      this.storeService.unread$
+        .pipe(
+          startWith(0),
+          switchMap(() => this.countReminders()),
+        ),
+    );


Q: Why do we need to reference unread notifications? 🤔

akabiru · 2024-12-06T11:05:34Z

frontend/src/app/features/work-packages/components/wp-reminder-modal/wp-reminder.modal.html

+    <turbo-frame
+      #frameElement


Nice one 👍🏾

lib/api/v3/reminders/reminders_api.rb

akabiru · 2024-12-06T11:09:54Z

spec/features/work_packages/reminders_spec.rb

+
+    describe "validations" do
+      it "renders errors on the date field or time field when the reminder is in the past" do
+        two_am = "02:00".to_time


akabiru · 2024-12-06T11:10:48Z

spec/features/work_packages/reminders_spec.rb

+      work_package_page.expect_no_reminder_button
+    end
+  end
+end


Nice work on the feature specs! 👍🏾

…tondialog-to-workpackage-page

jjabari-op · 2024-12-09T15:25:33Z

Thank you @akabiru for the comprehensive and detailed review! I'm on it! :)

…tondialog-to-workpackage-page

akabiru · 2024-12-10T18:21:30Z

@jjabari-op updating against dev as there were several recent CI stabalisation fixes that would be nice to have in

akabiru

🥇

akabiru · 2024-12-12T09:43:24Z

app/controllers/work_packages/reminders_controller.rb

+  # At the form level, we split the date and time into two form fields.
+  # In order to be a bit more informative of which field is causing
+  # the remind_at attribute to be in the past/invalid, we need to
+  # remap the error attribute to the appropriate field.
+  def prepare_errors_from_result(service_result)
+    # We set the reminder here for "create" case
+    # as the record comes from the service.
+    @reminder = service_result.result
+    @errors = service_result.errors
+
+    case @errors.find { |error| error.attribute == :remind_at }&.type
+    when :blank
+      handle_blank_error
+    when :datetime_must_be_in_future
+      handle_future_error
+    end
+
+    @errors.delete(:remind_at)
+  end


https://community.openproject.org/wp/60127

lib/api/v3/reminders/reminders_api.rb

jjabari-op added 2 commits December 3, 2024 16:19

[#59721] Add set/edit reminder button/dialog to workpackage page

b9f09cd

https://community.openproject.org/work_packages/59721

implemented reminder creation/update modal on workpackag page and bas…

8c08239

…ic spec coverage

jjabari-op self-assigned this Dec 3, 2024

jjabari-op added the pullpreview label Dec 3, 2024

jjabari-op requested a review from akabiru December 3, 2024 16:07

Base automatically changed from implementation/59433-define-database-model-for-reminders to dev December 4, 2024 14:50

Merge branch 'dev' into implementation/59721-add-setedit-reminder-but…

5dc51a9

…tondialog-to-workpackage-page

github-advanced-security bot found potential problems Dec 4, 2024

View reviewed changes

akabiru added this to the 15.2.x milestone Dec 4, 2024

akabiru added the feature label Dec 4, 2024

akabiru requested changes Dec 6, 2024

View reviewed changes

Merge branch 'dev' into implementation/59721-add-setedit-reminder-but…

d9133a2

…tondialog-to-workpackage-page

akabiru mentioned this pull request Dec 9, 2024

implementation/59434 visualize reminders in notification center #17323

Merged

8 tasks

Merge branch 'dev' into implementation/59721-add-setedit-reminder-but…

8de14b9

…tondialog-to-workpackage-page

jjabari-op added 5 commits December 11, 2024 12:21

addressed feedback from @akabiru

267dd6f

resolved more eslint errors

76159a3

adjust button text

c19dce0

fixed indentation on api schema defintion

84a5750

adding formerly unsaved changes

71fc03d

akabiru approved these changes Dec 12, 2024

View reviewed changes

akabiru merged commit 5ab30ac into dev Dec 12, 2024
16 checks passed

akabiru deleted the implementation/59721-add-setedit-reminder-buttondialog-to-workpackage-page branch December 12, 2024 09:57

github-actions bot removed the pullpreview label Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation/59721 add setedit reminder buttondialog to workpackage page #17341

Implementation/59721 add setedit reminder buttondialog to workpackage page #17341

jjabari-op commented Dec 3, 2024

akabiru left a comment •

edited

Loading

akabiru Dec 6, 2024

akabiru Dec 12, 2024

akabiru Dec 6, 2024

akabiru Dec 6, 2024 •

edited

Loading

akabiru Dec 6, 2024

akabiru Dec 6, 2024

akabiru Dec 6, 2024

akabiru Dec 6, 2024

akabiru Dec 6, 2024

akabiru Dec 6, 2024

jjabari-op commented Dec 9, 2024

akabiru commented Dec 10, 2024

akabiru left a comment

akabiru Dec 12, 2024

Implementation/59721 add setedit reminder buttondialog to workpackage page #17341

Implementation/59721 add setedit reminder buttondialog to workpackage page #17341

Conversation

jjabari-op commented Dec 3, 2024

akabiru left a comment • edited Loading

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

akabiru Dec 6, 2024 • edited Loading

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

jjabari-op commented Dec 9, 2024

akabiru commented Dec 10, 2024

akabiru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akabiru left a comment •

edited

Loading

akabiru Dec 6, 2024 •

edited

Loading