Skip to content

Fix for parameter "%n" on shared folders #64

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

Merged
merged 4 commits into from
Nov 25, 2020
Merged

Fix for parameter "%n" on shared folders #64

merged 4 commits into from
Nov 25, 2020

Conversation

k1l1
Copy link
Contributor

@k1l1 k1l1 commented Oct 16, 2020

Fix to allow '%n' parameter to give out real relative file path in shared folder scenario.
Addressing issues: #48 and possibly #38

Scenario

User A creates Folder /myfolder at shares it with User B
User B moves Folder from root dir to /subfolder/myfolder
User B creates file /subfolder/myfolder/file1 in this folder which triggers a workflow script:

Result before:
user-id (%o): A
nextcloud-relative path (%n): B/files/subfolder/myfolder/file1 <---- file does not exists on drive at this position

Result after:
user-id (%o): A
nextcloud-relative path (%n): B/files/myfolder/file1 <---- file exists

@k1l1 k1l1 requested a review from blizzz October 16, 2020 12:18
@k1l1 k1l1 changed the title Update Operation.php Fix for parameter "%n" on shared folders Oct 16, 2020
@Pingger
Copy link

Pingger commented Oct 16, 2020

Addressing issues: #48 and possibly #38

Issue #38 is not adressed.

} catch (InvalidPathException $e) {
} catch (NotFoundException $e) {
}
$config = new Config('config/');
Copy link
Member

Choose a reason for hiding this comment

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

this should come from \OCP\IConfig (method getSystemValue) and ought to be injected in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved. Can you check it?

$config = new Config('config/');
$base_path = $config->getValue('datadirectory');

$path = Filesystem::getLocalFile(Filesystem::getPath($nodeID));
Copy link
Member

Choose a reason for hiding this comment

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

$nodeID is not necessarily set, since the exceptions are suppressed and not handled further

Copy link
Contributor Author

@k1l1 k1l1 Oct 28, 2020

Choose a reason for hiding this comment

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

Hmm, what are the cases where $nodeID is not set and what is the best way to handle the exception? Should we fallback to the old solution (and have a possibly wrong file path on shared folders), or leave the %n path empty?

Copy link

Choose a reason for hiding this comment

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

@wiswedel
Copy link

@k1l1 Can you work with @blizzz 's feedback or do you need help resolving the open issues?

@k1l1
Copy link
Contributor Author

k1l1 commented Oct 27, 2020

I will try it myself this week and we'll see how it goes. I have to note that I have basically not much experience in php and the nextcloud environment, so please be gentle with me ;)

@wiswedel
Copy link

so please be gentle with me

Sure thing. Thanks for your contribution in the first place. Don't hesitate to ask if you're reaching dead ends. We're here to help.

@k1l1
Copy link
Contributor Author

k1l1 commented Oct 28, 2020

I can also confirm that creating and renaming files in group folders works and triggers a workflow_script
Is this the behaviour that is missing for you? @Pingger

Steps

  1. create user1 and user2 and set them to group standard
  2. Create groupfolder gf_test set it to group standard
  3. Create flow on create file When File created and FILE MIME type is PDF documents
  4. Upload pdf to gf_test/sample.pdf with user1, workflow script gives for %n now _groupfolders/1/sample.pdf
  5. Upload pdf to gf_test/sample2.pdf with user2, workflow script gives for %n now _groupfolders/1/sample2.pdf

The path _groupfolders/1/sample.pdf exists in nextclouds data folder!

@k1l1 k1l1 requested a review from blizzz November 5, 2020 09:01
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

Looks good and does what is advertized :)

I added another commit fixing whitespaces – we use tabs not spaces

@blizzz
Copy link
Member

blizzz commented Nov 24, 2020

@blizzz
Copy link
Member

blizzz commented Nov 24, 2020

and apologies for my late reaction, the notification drowned among others :(

k1l1 and others added 4 commits November 25, 2020 13:15
Fix to allow '%n' parameter to give out real relative file path in shared folder scenario.

Signed-off-by: Kilian Pfeiffer <k.pfeiffer@pfffer.eu>
Signed-off-by: Kilian Pfeiffer <k.pfeiffer@pfffer.eu>
Signed-off-by: Kilian Pfeiffer <k.pfeiffer@pfffer.eu>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Kilian Pfeiffer <k.pfeiffer@pfffer.eu>
@k1l1 k1l1 force-pushed the fix_shared_folder branch from 45c2ac4 to ba30955 Compare November 25, 2020 12:15
@k1l1 k1l1 merged commit 6c33adf into master Nov 25, 2020
@k1l1
Copy link
Contributor Author

k1l1 commented Nov 25, 2020

merged!

@delete-merged-branch delete-merged-branch bot deleted the fix_shared_folder branch November 25, 2020 12:16
@blizzz
Copy link
Member

blizzz commented Nov 25, 2020

/backport to stable20

@blizzz
Copy link
Member

blizzz commented Nov 25, 2020

/backport to stable19

@blizzz
Copy link
Member

blizzz commented Nov 25, 2020

/backport to stable18

@jakobroehrl
Copy link

@blizzz When do you plan to relase this fix?
Would love to test it :)

@wiswedel
Copy link

@blizzz When do you plan to relase this fix?
Would love to test it :)

@jakobroehrl release is out, thanks for your patience: https://github.com/nextcloud/workflow_script/releases/tag/v1.3.3

@jakobroehrl
Copy link

Thanks for the information.
Tested it already and it works great! :)

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.

5 participants