Feature pushed to PR #621
Feature pushed to PR #621
Conversation
| @@ -24,6 +24,7 @@ | |||
| "stylelint:fix": "stylelint 'css/*.css' 'css/*.scss' 'src/**/*.scss' 'src/**/*.vue' --fix" | |||
| }, | |||
| "dependencies": { | |||
| "16": "^0.0.2", | |||
There was a problem hiding this comment.
Why is this dependency required?
There was a problem hiding this comment.
Actually, I was facing the npm version errors while running the npm ci or npm build. The table view wasn't loading due to these errors. That's the reason for the above change.
There was a problem hiding this comment.
Does that mean the dependency isn't required?
There was a problem hiding this comment.
Yeah this should not be needed, can you drop the changes from package.json/package-lock.json from the PR?
There was a problem hiding this comment.
SuperService does not make it clear what the service does.
| @@ -16,4 +16,39 @@ public function __construct(LoggerInterface $logger, ?string $userId, Permission | |||
| $this->logger = $logger; | |||
| $this->userId = $userId; | |||
| } | |||
|
|
|||
| public function AdminNotificationCall($tableId,$viewId) { | |||
There was a problem hiding this comment.
AdminNotificationCall is not descriptive.
| $conditionalTerm = (!is_null($tableId)) ? 'Table Id '.$tableId: 'View Id '.$viewId; | ||
|
|
||
| $data = array( | ||
| 'shortMessage' => 'Alert !', |
There was a problem hiding this comment.
I can't find the feature of language switching the in application. If you want to develop the functionality like that I can do it.
There was a problem hiding this comment.
Language switching is built into Nextcloud. It can be implemented using the t JavaScript function, which replaces the text with text translated in the chosen language. The language files are located in the l10n directory.
An example is line 56 in src/store/data.js.
} catch (e) {
displayError(e, t('tables', 'Could not load columns.'))
return false
}If the user uses Arabic it will load the text from l10n/ar.js at line 431:
"Could not load columns." : "تعذر تحميل الأعمدة.",There was a problem hiding this comment.
It would require to inject the IL10N service, further reference can be found at https://docs.nextcloud.com/server/latest/developer_manual/basics/front-end/l10n.html#php
|
|
||
| // Check for cURL errors | ||
| if (curl_errno($ch)) { | ||
| echo 'cURL error: ' . curl_error($ch); |
There was a problem hiding this comment.
This should use the dedicated logger.
| $data); | ||
| $data); | ||
|
|
||
| $superService->AdminNotificationCall($tableId,$viewId); |
There was a problem hiding this comment.
It would be better to trigger the notification within the service class to also cover other code paths outside of the API controllers to emit a notification.
| ): DataResponse { | ||
| return $this->handleError(function () use ($tableId, $viewId, $data, $superService) { | ||
|
|
||
| $create = $this->service->create( |
There was a problem hiding this comment.
For some more obvious naming
| $create = $this->service->create( | |
| $newRow = $this->service->create( |
|
|
||
| public function AdminNotificationCall($tableId,$viewId) { | ||
|
|
||
| $url = 'http://admin:admin@localhost/ocs/v2.php/apps/notifications/api/v2/admin_notifications/admin'; |
There was a problem hiding this comment.
This is not the expected API to be used, the reference is about https://github.com/nextcloud/notifications/blob/master/docs/notification-workflow.md which is a PHP API provided by Nextcloud server. There is no need to perform an HTTP request here especially with passing username/password.
| $conditionalTerm = (!is_null($tableId)) ? 'Table Id '.$tableId: 'View Id '.$viewId; | ||
|
|
||
| $data = array( | ||
| 'shortMessage' => 'Alert !', |
There was a problem hiding this comment.
It would require to inject the IL10N service, further reference can be found at https://docs.nextcloud.com/server/latest/developer_manual/basics/front-end/l10n.html#php
| curl_close($ch); | ||
|
|
||
| // Output the response | ||
| return $response; |
There was a problem hiding this comment.
Is there a need to return that?
| @@ -16,4 +16,39 @@ public function __construct(LoggerInterface $logger, ?string $userId, Permission | |||
| $this->logger = $logger; | |||
| $this->userId = $userId; | |||
| } | |||
|
|
|||
| public function AdminNotificationCall($tableId,$viewId) { | |||
| @@ -24,6 +24,7 @@ | |||
| "stylelint:fix": "stylelint 'css/*.css' 'css/*.scss' 'src/**/*.scss' 'src/**/*.vue' --fix" | |||
| }, | |||
| "dependencies": { | |||
| "16": "^0.0.2", | |||
There was a problem hiding this comment.
Yeah this should not be needed, can you drop the changes from package.json/package-lock.json from the PR?
|
Hello there, We hope that the reviewing process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR reviewing process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
|
@juliushaertl Are here any further plans? Otherwise I would like to close this one as it does not follow our idea to implement a flow trigger that can be used to setup notifications in a more flexible way?! #552 |
|
Yes, when integrating the notifications app this would also not work as we'd need to use the PHP OCP API for emitting the notifications |
No description provided.