Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OP#42323] mirror OP notifications to NC notifications #256

Merged
merged 19 commits into from
Nov 9, 2022

Conversation

individual-it
Copy link
Collaborator

@individual-it individual-it commented Oct 17, 2022

part of https://community.openproject.org/projects/nextcloud-integration/work_packages/42323

All current notifications of OpenProject should be mirrored to Nextcloud.
The view is similar to the notification center in OpenProject where the notifications are aggregated by work-package.

@individual-it individual-it changed the title mirror OP notifications to NC notifications [OP#42323] mirror OP notifications to NC notifications Oct 17, 2022
@individual-it
Copy link
Collaborator Author

@julien-nc your idea about how to react on the dismiss button works great, but now I have a new problem.
Every time I check for new notifications I want to get rid of all old ones, so I call markProcessed() in https://github.com/nextcloud/integration_openproject/pull/256/files#diff-acd9bb903dbec989fbe06b290cfcaed634a3f67d497845d4d6946fa1b6449c93R173
That itself calls the function to mark the notifications as read in OP.
I cannot find a way of distinguish between the user clicking the dismiss button and a call from markProcessed

@individual-it individual-it force-pushed the mirrorOPNotifications branch 3 times, most recently from 41f704f to eceeec0 Compare October 21, 2022 06:49
@individual-it
Copy link
Collaborator Author

using now OCA\Notifications\Handler::deleteIds() to delete existing notifications before recreating them. This function does not trigger the dismissNotification() hook so its ideal for us, but it was first introduced in NC 23 (nextcloud/notifications#1156), so using that function would mean dropping support for NC 22

@individual-it
Copy link
Collaborator Author

deleteIds() was not the right solution, because it would not dismiss notifications on other connected devices like Android.
Current solution is to set a marker for the time when the code dismisses the existing notifications.

@individual-it
Copy link
Collaborator Author

I've added more logic to remember the latest updatedAt field of all notifications associated with a WP, using that we don't have to dismiss all existing notifications and recreate them. This makes mobile clients only beep when something changed on the OP side

@kiranparajuli589
Copy link
Contributor

One case that I found:

Steps to reproduce

  1. Connect a normal user (not admin) from NC with the OP server.
  2. Make notifications for that user from OP
  3. Come to NC dashboard (login with the same normal user)
  4. Open the NC notification dropdown and click the (x) icon.

Expected

  1. mark read should succeed in the NC
  2. mark read should synchronize to OP

Actual

  1. mark read fails with `` in NC
    Request:
    {
        "DELETE": {
    	    "scheme": "http",
    	    "host": "localhost",
    	    "filename": "/nextcloud/master/apps/integration_openproject/work-packages/4/notifications",
    	    "remote": {
    		    "Address": "127.0.0.1:80"
    	    }
        }
    }
    Response:
    {
        "message": "Logged in user must be an admin"
    }
  2. OP detects no change

@individual-it
Copy link
Collaborator Author

@kiranparajuli589 this is even a problem in the current master branch, fixed in #262
Please test first there and after its merged I will rebase this PR

@kiranparajuli589
Copy link
Contributor

#262 merged.

@individual-it
Copy link
Collaborator Author

@kiranparajuli589 I've rebased and pushed this branch

Copy link
Contributor

@kiranparajuli589 kiranparajuli589 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Success:

  • marking notification as read from the app :
    • from the notifications panel of mobile: syncs with OP & NC web
    • the notification center of the app itself: syncs with OP & NC web
  • there is existing unread notification, and a new is added for the same wp
    • new notification makes a beep on the phone...also syncs with NC web and OP web
  • there are existing unread notifications and cron job is rerun.
    • things are the way it was.

One thing that was not working with me:

  • When I get notifications in NC and I mark them as read from the Dashboard widget...it gets' cleared from the OP but the NC notification are still there.

@individual-it
Copy link
Collaborator Author

@kiranparajuli589 I can mark notifications in the dashboard as read and they disappear, see attached video
can you please provide me steps to reproduce the issue

2022-11-08_12-08-39.mp4

@individual-it
Copy link
Collaborator Author

One problem that I've found is that when ALL notifications are marked as read the code does not execute, because I check if count($notifications) > 0. I will fix that

Signed-off-by: Artur Neumann <artur@jankaritech.com>
… read

Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
This reverts commit eceeec0.

Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

JS Code Coverage

Coverage after merging mirrorOPNotifications into master will be
93.73%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   adminSettings.js0%0%0%0%1, 1, 10–19, 2, 20–25, 3–9
   bootstrap.js0%0%0%0%1, 1–7
   dashboard.js0%0%0%0%1, 1, 10–19, 2, 20–27, 3–9
   fileActions.js0%0%0%0%1, 1, 10–17, 2–9
   personalSettings.js0%0%0%0%1, 1, 10–19, 2, 20–25, 3–9
   projectTab.js0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–66, 7–9
   utils.js57.45%33.33%50%59.52%10–14, 17–26, 6–9
src/components
   AdminSettings.vue99.08%95.83%77.78%99.86%1, 1, 1
   OAuthConnectButton.vue99.08%80%100%100%1
   PersonalSettings.vue97.81%91.67%71.43%99.39%1, 1
src/components/admin
   FieldValue.vue95.56%62.50%100%98.77%1, 1, 1, 1
   FormHeading.vue98.98%75%100%100%1
   TextInput.vue98.52%80%87.50%100%1, 1, 1
src/components/icons
   ClippyIcon.vue93.18%50%50%97.50%1, 1
src/components/settings
   CheckBox.vue92.45%80%66.67%97.62%1, 1
   SettingsTitle.vue95.56%50%100%97.62%1, 1
src/components/tab
   EmptyContent.vue99.34%90.91%100%100%1
   SearchInput.vue99.56%83.33%100%100%1
   WorkPackage.vue99.01%33.33%100%99.66%1, 1, 1
src/utils
   workpackageHelper.js97.46%96%100%97.75%17–19
src/views
   Dashboard.vue98.20%50%50%99.63%1, 1, 1
   ProjectsTab.vue99.74%92.31%100%100%23

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

PHP Code Coverage

Coverage after merging mirrorOPNotifications into master will be
63.26%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
lib/AppInfo
   Application.php20.69%100%25%20%106, 110, 61–62, 65, 72, 74–76, 81–84, 86, 90–95
lib/BackgroundJob
   CheckNotifications.php0%100%0%0%48, 50, 52–53, 61–62
lib/Controller
   ConfigController.php79.43%100%66.67%80.30%111–112, 114, 116–118, 120, 126–127, 129, 319–322, 324–325, 328, 336–340, 362–364, 94
   DirectDownloadController.php0%100%0%0%36–38, 53–55, 57, 60–61
   FilesController.php73.68%100%100%71.91%168, 221–222, 267, 273–277, 281–282, 285, 287, 298–300, 303–304, 306–307, 311–314, 317
   OpenProjectAPIController.php84.92%100%78.57%85.41%136, 171, 188–189, 193, 197, 199–204, 206, 215–216, 219, 221, 223–226, 228–229, 234, 253, 278, 95
lib/Dashboard
   OpenProjectWidget.php0%100%0%0%101–102, 104–108, 116, 123–124, 126, 128–129, 131–132, 134, 137–138, 140–141, 143, 69–73, 80, 87, 94
lib/Exception
   OpenprojectErrorException.php100%100%100%100%
   OpenprojectResponseException.php100%100%100%100%
lib/Listener
   LoadSidebarScript.php0%100%0%0%100–101, 103–104, 106, 108, 65–70, 72–73, 75–76, 78–79, 85–86, 88, 90, 92–94, 96, 98
lib/Notification
   Notifier.php0%100%0%0%101, 103–106, 109–110, 113, 120–122, 124–125, 127–128, 130, 132–135, 141–142, 144, 146, 155–156, 158–159, 164–168, 61–66, 76, 85, 96, 98
lib/Search
   OpenProjectSearchProvider.php0%100%0%0%102, 109–110, 113–116, 118–119, 123–125, 127–128, 130–131, 133–135, 138–139, 141–142, 146–147, 149–151, 157–158, 160, 169, 177–183, 192–197, 206, 71–75, 82, 89, 97, 99
   OpenProjectSearchResultEntry.php100%100%100%100%
lib/Service
   DirectDownloadService.php88%100%100%86.96%65–66, 68
   OauthService.php0%100%0%0%101–105, 115–118, 42–44, 53–59, 61, 70–73, 84–91
   OpenProjectAPIService.php86.97%100%88.46%86.84%148, 175, 203, 233–237, 266–270, 433–434, 436, 450–451, 460, 464, 485, 573–574, 581, 584–587, 589, 595, 599–601, 773, 844, 874–875, 877, 879–880
lib/Settings
   Admin.php0%100%0%0%32–34, 41–43, 46–50, 53, 58–60, 63, 65–66, 68, 72, 76
   AdminSection.php0%100%0%0%19–20, 29, 39, 48, 55
   Personal.php88.89%100%50%93.75%101, 105
   PersonalSection.php0%100%0%0%19–20, 29, 39, 48, 55

@kiranparajuli589
Copy link
Contributor

kiranparajuli589 commented Nov 9, 2022

@kiranparajuli589 I can mark notifications in the dashboard as read and they disappear, see attached video can you please provide me steps to reproduce the issue

I think I missed to run the cron-job after marking as read from the dashboard ;) everything works gr8. sorry for this inconvenience.

@individual-it individual-it merged commit 8e60fcb into master Nov 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the mirrorOPNotifications branch November 9, 2022 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants