Skip to content

Add share link entry #5

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.

Sign up for GitHub

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

Jump to bottom
Merged
skjnldsv merged 4 commits into from
Sep 16, 2021
Merged

Add share link entry #5

skjnldsv merged 4 commits into from
Sep 16, 2021

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Sep 10, 2021 •
edited
Loading

@skjnldsv skjnldsv marked this pull request as draft September 10, 2021 14:32
@skjnldsv skjnldsv force-pushed the feat/front-menu-entry branch from 5115c77 to 05568aa Compare September 14, 2021 15:40
@skjnldsv
Add share link entry
32ae1db 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv skjnldsv force-pushed the feat/front-menu-entry branch from 05568aa to 32ae1db Compare September 14, 2021 15:45
@skjnldsv skjnldsv marked this pull request as ready for review September 14, 2021 15:45
julien-nc
julien-nc reviewed Sep 14, 2021
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Code looks good. Too bad we can't try it because ExternalShareAction is not allowed inside the Actions component.

One question though: Is the DownloadLimitAction displayed even if the limit is disabled?

Pytal
Pytal reviewed Sep 15, 2021
View reviewed changes
.eslintrc.js
@@ -1,3 +1,6 @@
module.exports = {
globals: {
appName: true,
Copy link
Member

@Pytal Pytal Sep 15, 2021

Choose a reason for hiding this comment

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

After checking the docs https://eslint.org/docs/user-guide/configuring/language-options#using-configuration-files-1 shouldn't this be set to 'readonly'?

@skjnldsv skjnldsv mentioned this pull request Sep 15, 2021
Merged
@skjnldsv skjnldsv added the enhancement New feature or request label Sep 15, 2021
@julien-nc julien-nc mentioned this pull request Sep 15, 2021
Closed
@PVince81
Copy link
Member

PVince81 commented Sep 15, 2021

  • add README with notes about how to set the counter as I couldn't find it

@PVince81
Copy link
Member

PVince81 commented Sep 15, 2021

or it's a bug... I'm currently on server master 995aa6518357b9f371ced5ba8941dfd2a02b0d97 and this PR.

I can't see any option to set the download limit:

image

also I didn't find any option in the admin's share settings, in case the admin needs to enable it.

so either it's not working or it's well hidden ?

@PVince81
Copy link
Member

PVince81 commented Sep 15, 2021

@skjnldsv
Add public message
4e1befd 
Signed-off-by: John Molakvoæ <[email protected]>
@PVince81 PVince81 mentioned this pull request Sep 15, 2021
Closed
PVince81
PVince81 reviewed Sep 15, 2021
View reviewed changes
lib/Listener/ShareLinkAccessedListener.php
$count = $this->config->getAppValue(Application::APP_ID, 'download_count_' . $token, 0) + 1;
$this->config->setAppValue(Application::APP_ID, 'download_count_' . $token, $count);
// Increment this download event
$downloads = $shareLimit->getDownloads() + 1;
Copy link
Member

@PVince81 PVince81 Sep 15, 2021

Choose a reason for hiding this comment

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

potential concurrency concern, I've raised it #8 as it doesn't look that critical and rather unlikely to be triggered by a regular user

PVince81
PVince81 approved these changes Sep 15, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 code looks fine to me so far

I was not able to test due to dependencies.
Also see comment + ticket about potential concurrency concern.

@PVince81
Copy link
Member

PVince81 commented Sep 15, 2021

I guess we shouldn't merge until this actually works after resolving the related PRs

@szaimen
Copy link
Contributor

szaimen commented Sep 15, 2021

I can't see any option to set the download limit:

image

also I didn't find any option in the admin's share settings, in case the admin needs to enable it.

so either it's not working or it's well hidden ?

Same for me.

Tested with 
docker run -d \
-e SERVER_BRANCH=feat/ExternalShareActions-API \
-e DOWNLOADLIMIT_BRANCH=feat/front-menu-entry \
-p 8443:443 \
-e TRUSTED_DOMAIN=192.168.178.34 \
--name nextcloud-easy-test \
--user=www-data \
ghcr.io/szaimen/nextcloud-easy-test:latest

@julien-nc
Copy link
Member

julien-nc commented Sep 15, 2021 •
edited
Loading

@PVince81 @szaimen Even if my solution probably won't be used, it allows to test this PR. Just use server's feat/ExternalShareActions-API-bis branch and you'll be able to see the sharing entries.

The current problem with the feat/ExternalShareActions-API branch is that the Actions component refuses to contain something else than Action components (ActionLink, ActionCheckbox etc...). So the ExternalShareAction component is not displayed/rendered inside Actions. You should see an error in the browser logs.

@szaimen
Copy link
Contributor

szaimen commented Sep 15, 2021

Just use server's feat/ExternalShareActions-API-bis branch and you'll be able to see the sharing entries.

Can confirm that it shows up with your branch but that has its own problems... So probably we need to decide first which attempt to use and then test it and fix the issues there...

@PVince81
Merge pull request #7 from nextcloud/feat/public
c39bbee 
Add limit message on public page
Pytal
Pytal approved these changes Sep 15, 2021
View reviewed changes
@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 16, 2021

nextcloud-libraries/nextcloud-vue#2252

@skjnldsv
Adjust public wording
c752e9f 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv skjnldsv merged commit 8e79206 into master Sep 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/front-menu-entry branch September 16, 2021 13:44
@i2h3 i2h3 mentioned this pull request Nov 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julien-nc julien-nc julien-nc left review comments

@PVince81 PVince81 PVince81 approved these changes

@blizzz blizzz blizzz approved these changes

@Pytal Pytal Pytal approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unshare public link after X explicit downloads

7 participants

@skjnldsv @PVince81 @szaimen @julien-nc @blizzz @Pytal