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

NC 18 Flow integration #10

Closed
Rello opened this issue Dec 15, 2019 · 27 comments
Closed

NC 18 Flow integration #10

Rello opened this issue Dec 15, 2019 · 27 comments
Labels
enhancement Feature request
Milestone

Comments

@Rello
Copy link
Owner

Rello commented Dec 15, 2019

Hello @blizzz, @juliushaertl

I would like to follow up our conversation regarding Workflow.
My last status was, that you were still working several basic classes.

  • Do you have any update for me if it is worth starting already to provide an Operation?
  • Can you direct me towards a reference app or how-to? (you showed my some prototypes back then)

thank you,
Rello

@Rello Rello added the enhancement Feature request label Dec 15, 2019
@Rello Rello mentioned this issue Dec 15, 2019
31 tasks
@juliushaertl
Copy link
Contributor

Do you have any update for me if it is worth starting already to provide an Operation?

The API for 18 is supposed to stay like this as we are post feature freeze now.

Can you direct me towards a reference app or how-to? (you showed my some prototypes back then)

There is no howto or documentation yet unfortunately, but for a reference implementation nextcloud/spreed#2487 is probably the best for a basic operation (the others files_accesscontrol/files_automatedtagging are more special cases)

@blizzz
Copy link

blizzz commented Dec 18, 2019

@Rello There's also the talk @juliushaertl and I gave at the conf. The basics are the same, specifics have changed a little. Otherwise, yes, the Talk implementation @juliushaertl mentioned is the latest app, that got a very new implementation and this is a great reference.

@blizzz
Copy link

blizzz commented Dec 18, 2019

@Rello btw, when you were here we were talking about a Timer like event. In fact, it's probably easier (UX wise, too) if you solve it with a background job.

@Rello
Copy link
Owner Author

Rello commented Dec 18, 2019

@blizzz it would require a complete new UX for the cron job customisation.
for Flow, the Action UI is already there and everything would be in one place. Now I would do an operation for flow and another implementation for a local UI which would be similar

I would like the flow part. perhaps I will look at it if I have fee time

@Rello
Copy link
Owner Author

Rello commented Jan 3, 2020

Hello @blizzz @juliushaertl
when creating a flow I am missing a filter for the mime type. I think we talked about it a while back.
can this be added on server side? or would a custom implementation be required?

I think it would make it easier to have it as a filter. without I would hand over every added file to my operator and check for the mime-type there. but the initial trigger sounds more reasonable to me

@blizzz
Copy link

blizzz commented Jan 6, 2020

@Rello the event is being thrown and caught in any case. So, basically check it in your onEvent implementation and exit early if it is not relevant. The PDF converter does it similar: https://github.com/nextcloud/workflow_pdf_converter/blob/master/lib/Operation.php#L87-L108

Rello added a commit that referenced this issue Jan 6, 2020
@Rello
Copy link
Owner Author

Rello commented Jan 6, 2020

Hello @blizzz
I am trying a very basic setup to start with the flow-tile.

  • registered in application.php
  • created a lib/Flow/Operation.php
  • created an main.js with the frontend registration

but it is not showing up.
can you check this commit and let me know what I am missing?

Rello added a commit that referenced this issue Jan 6, 2020
@blizzz
Copy link

blizzz commented Jan 7, 2020

@Rello Timing. When you do not instantiate your Application instance yourself, it will happen too late. Also, it is better to keep logic out of the constructor. This does:

diff --git a/appinfo/app.php b/appinfo/app.php
index 539c8bf..bff433f 100644
--- a/appinfo/app.php
+++ b/appinfo/app.php
@@ -14,6 +14,9 @@ namespace OCA\Analytics\AppInfo;
 
 use OCP\Util;
 
+$app = \OC::$server->query(Application::class);
+$app->register();
+
 $navigationEntry = function () {
     return [
         'id' => 'analytics',
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 9594c26..7cb6ec6 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -25,7 +25,6 @@ class Application extends App
     {
 
         parent::__construct('analytics', $urlParams);
-        $this->register();
     }
 
     public function register()

@Rello
Copy link
Owner Author

Rello commented Jan 7, 2020

Screenshot 2020-01-07 at 16 48 38

sweet. thanks!
I thought about it. I think I might move it to a separate flow app to reduce the complexity of DA itself.

@Rello
Copy link
Owner Author

Rello commented Jan 7, 2020

one more question:
you are using VUE.
is there a chance to provide the details (i need a multiselect like in the spreed operation) via vanilla JS?
I don´t know about vue and I will not have time to work into it

@blizzz
Copy link

blizzz commented Jan 8, 2020

@juliushaertl ^ is this possible?

@Rello
Copy link
Owner Author

Rello commented Jan 8, 2020

alternatively I could need your help because I don´t have the vue environment.
I just need someone to create the final JS file.
Functionality will be the same as spreed: just one multiselect which calls one url, that returns a report (id + text)

@juliushaertl
Copy link
Contributor

Plain javascript would be possible with some vue component boilerplate code (similar to https://github.com/nextcloud/workflow_script/blob/master/js/admin.js) so you won't need any vue dependencies in your app. I can help out and provide you with an example for that. Otherwise I can also prepare a pull request for a vue implementation so that the app also uses the default multiselect component, but might need to wait until next week. Up to you. 😉

@Rello
Copy link
Owner Author

Rello commented Jan 8, 2020

I like that idea. seams reasonable for this very basic requirement
Screenshot 2020-01-08 at 19 07 47

So I guess the questions are:

I am trying to compare against
https://github.com/nextcloud/spreed/blob/e74e89fcb70d8c59cbc0d1ab93d0118640280582/src/views/FlowPostToConversation.vue

Rello added a commit that referenced this issue Jan 8, 2020
Rello added a commit that referenced this issue Jan 8, 2020
@Rello
Copy link
Owner Author

Rello commented Jan 8, 2020

ok, I can create a dropdown - select a value - and the operation value is transmitted!
Screenshot 2020-01-08 at 20 47 08
Screenshot 2020-01-08 at 20 39 53

and it arrives in the database
Screenshot 2020-01-08 at 20 40 27

Rello added a commit that referenced this issue Jan 8, 2020
Rello added a commit that referenced this issue Jan 8, 2020
@Rello
Copy link
Owner Author

Rello commented Jan 8, 2020

OK, it works!
thank you @juliushaertl

If I may ask for some minutes: could you have quick review of my flow.js if there are any no-gos or if it looks acceptable? thank you very much

@Rello Rello added the testing development finished; in testing label Jan 8, 2020
@Rello Rello added this to the 2.0.0 milestone Jan 9, 2020
Rello added a commit that referenced this issue Jan 9, 2020
@juliushaertl
Copy link
Contributor

If I may ask for some minutes: could you have quick review of my flow.js if there are any no-gos or if it looks acceptable? thank you very much

Looks sane to me.

When testing, i encountered that your Application class is not loaded in app.php, so the operation is not registered. Please see the comment from @blizzz for that.

Rello added a commit that referenced this issue Jan 9, 2020
@Rello
Copy link
Owner Author

Rello commented Jan 9, 2020

yes. sorry for that. did not commit the app/application to this ticket
works now
i am just finalising the operation.php

I think for documentation purpose, there is a lot of topics to write up e.g. like the way from $event to the fileid or path

currently its a lot of try/error/debug for an external

$node = $event->getSubject()
$node->getPath()

or

$flows = $ruleMatcher->getFlows(false);
foreach ($flows as $flow) {
     $flow['operation']
 }

Rello added a commit that referenced this issue Jan 9, 2020
@Rello
Copy link
Owner Author

Rello commented Jan 9, 2020

its working
thank you both for your support

flow

Wiki:
https://github.com/Rello/analytics/wiki/Flow-integration

Rello added a commit that referenced this issue Jan 9, 2020
@Rello Rello changed the title NC 18 Workflow integration NC 18 Flow integration Jan 9, 2020
@blizzz
Copy link

blizzz commented Jan 10, 2020

@Rello btw, you can add it to the flow category, too:

diff --git a/appinfo/info.xml b/appinfo/info.xml
index d32c6b4..5aa1ea4 100644
--- a/appinfo/info.xml
+++ b/appinfo/info.xml
@@ -22,6 +22,7 @@ From data to report. Data Analytics provides modular datasources, a realtime or
    <author>Marcel Scherello</author>
    <namespace>Analytics</namespace>
     <category>office</category>
+    <category>workflow</category>
     <website>https://github.com/rello/analytics</website>
     <bugs>https://github.com/rello/analytics/issues</bugs>
     <repository type="git">https://github.com/rello/analytics.git</repository>

@blizzz
Copy link

blizzz commented Jan 10, 2020

I think for documentation purpose, there is a lot of topics to write up e.g. like the way from $event to the fileid or path

Yes, this is certainly true

@Rello
Copy link
Owner Author

Rello commented Jan 11, 2020

Hello Masters,
can you help me once more? DA breaks on NC17
Undefined class constant 'EVENT_NAME_REG_OPERATION

what check could I put in application.php to register only on NC18?

@blizzz
Copy link

blizzz commented Jan 13, 2020

did you try to wrap it in a if class_exists()?

@Rello
Copy link
Owner Author

Rello commented Jan 13, 2020

the class is existing. I ended up using
if (property_exists('IManager', 'EVENT_NAME_REG_OPERATION')) {
now the app also works in NC17

@Rello
Copy link
Owner Author

Rello commented Jan 14, 2020

Hello @blizzz I still need help.
this did not work - not showing in NC18 anymore
I tried class_exists('IRuleMatcher') because this came in NC18 - also not working in NC18.

another suggestion?

@blizzz
Copy link

blizzz commented Jan 14, 2020

I take a closer look today

@blizzz
Copy link

blizzz commented Jan 14, 2020

@Rello this works:

diff --git a/lib/Flow/Operation.php b/lib/Flow/Operation.php
index 0ab2f4e..25686f5 100644
--- a/lib/Flow/Operation.php
+++ b/lib/Flow/Operation.php
@@ -52,11 +52,13 @@ class Operation implements IOperation
 
     public static function register(IEventDispatcher $dispatcher): void
     {
-        $dispatcher->addListener(IManager::EVENT_NAME_REG_OPERATION, function (GenericEvent $event) {
-            $operation = \OC::$server->query(Operation::class);
-            $event->getSubject()->registerOperation($operation);
-            Util::addScript('analytics', 'flow');
-        });
+       if(interface_exists(IRuleMatcher::class)) {
+           $dispatcher->addListener(IManager::EVENT_NAME_REG_OPERATION, function (GenericEvent $event) {
+               $operation = \OC::$server->query(Operation::class);
+               $event->getSubject()->registerOperation($operation);
+               Util::addScript('analytics', 'flow');
+           });
+       }
     }
 
     public function getDisplayName(): string
@@ -116,4 +118,4 @@ class Operation implements IOperation
             return;
         }
     }
-}
\ No newline at end of file
+}

Rello added a commit that referenced this issue Jan 14, 2020
@Rello Rello added pending release part of the next release version and removed testing development finished; in testing pending release part of the next release version labels Jan 15, 2020
@Rello Rello closed this as completed Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request
Projects
None yet
Development

No branches or pull requests

3 participants