Fix public handling of limited shares #76

skjnldsv · 2022-04-08T13:35:12Z

Reset the counter when setting a new limit
Display a message and hide the button when limit is reached and share disabled
Display a warning when clicking the download button more than once
Prevent mouse events on shared images
Adding loading state

Fix #43
Fix #8

Signed-off-by: John Molakvoæ <[email protected]>

szaimen · 2022-04-08T14:20:22Z

@skjnldsv Maybe it would make sense to prevent all right-click options when the counter is at null?
See nextcloud/server#31372 as inspiration

skjnldsv · 2022-04-08T15:05:23Z

@skjnldsv Maybe it would make sense to prevent all right-click options when the counter is at null? See nextcloud/server#31372 as inspiration

I would even do that even if the counter is not null as there is the download button already.
Thanks for the idea! 👍

szaimen · 2022-04-08T15:06:19Z

I would even do that even if the counter is not null as there is the download button already.
Thanks for the idea! 👍

sounds good! you are welcome :)

Signed-off-by: John Molakvoæ <[email protected]>

skjnldsv · 2022-04-08T15:10:01Z

Done, please review 🙏

szaimen

apart from that looks good to me but didn't test it

src/public.js

szaimen

Testing reveals that saving the limit does not close the popup which feels strange:

Also Using this button does not update the download count on top of the page automatically:

szaimen

another thing: when reaching the download limit, the download button on the bottom should probably removed as well:

skjnldsv · 2022-04-12T08:15:09Z

Testing reveals that saving the limit does not close the popup which feels strange:
image

Same for all the other options i the sharing menu, this would have to be in https://github.com/nextcloud/server/blob/da435b1e67930e85fc30fd1b94c6214caa086f4f/apps/files_sharing/src/components/SharingEntryLink.vue#L128-L132

Signed-off-by: John Molakvoæ <[email protected]>

skjnldsv · 2022-04-12T08:25:21Z

All other issues addressed, please review @szaimen :)

szaimen · 2022-04-12T08:50:13Z

All other issues addressed, please review @szaimen :)

on it

szaimen

async saving is already a bit better than no saving feedback.
But this leads to a problem when deleting the already set value:

szaimen · 2022-04-12T08:54:00Z

I can confirm that all other issues are fixed :)

skjnldsv · 2022-04-12T08:54:06Z

But this leads to a problem when deleting the already set value:

Will be fixed in followup then, to support null and 0 values (see #69, wwhich I will re-adapt)

szaimen · 2022-04-12T08:54:26Z

Will be fixed in followup then, to support null and 0 values (see #69, wwhich I will re-adapt)

okay

szaimen

good to go from my side then :)

skjnldsv · 2022-04-12T08:58:14Z

But this leads to a problem when deleting the already set value:

Will be fixed in followup then, to support null and 0 values (see #69, wwhich I will re-adapt)

#82

come-nc · 2022-04-12T09:00:31Z

src/public.js

+let count = limit - downloads
+let clicks = 0
+
+const updateCounter = function(span) {


missing count parameter no?

come-nc · 2022-04-12T09:01:04Z

src/public.js


 		span.style = 'color: var(--color-primary-text); padding: 0 10px;'
-		span.innerText = n('files_downloadlimit', '1 remaining download allowed', '{count} remaining downloads allowed', count, { count })
+		updateCounter(span, count)


or extra count parameter here, not clear, it seems count is actually global

Yes, it's cleaner, you're right, let me fix in a pr :)

Fix public handling of limited shares

27fc734

Signed-off-by: John Molakvoæ <[email protected]>

skjnldsv requested review from a team, PVince81, szaimen and vanpertsch and removed request for a team April 8, 2022 13:51

skjnldsv self-assigned this Apr 8, 2022

skjnldsv requested review from CarlSchwan, artonge and come-nc April 8, 2022 13:57

skjnldsv added enhancement New feature or request 3. to review Waiting for reviews labels Apr 8, 2022

Prevent context menu

c533ed4

Signed-off-by: John Molakvoæ <[email protected]>

szaimen approved these changes Apr 8, 2022

View reviewed changes

src/public.js Outdated Show resolved Hide resolved

szaimen requested changes Apr 8, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

szaimen requested changes Apr 8, 2022

View reviewed changes

szaimen mentioned this pull request Apr 8, 2022

Discussion if disabling share after limit is reached is expected #80

Closed

artonge approved these changes Apr 11, 2022

View reviewed changes

Fix multiple downloads buttons and disabling state

c245c09

Signed-off-by: John Molakvoæ <[email protected]>

skjnldsv requested a review from szaimen April 12, 2022 08:24

skjnldsv mentioned this pull request Apr 12, 2022

fixed the issue to accept 0 and null and changed the default text value to remaining downloads #69

Closed

szaimen requested changes Apr 12, 2022

View reviewed changes

szaimen approved these changes Apr 12, 2022

View reviewed changes

skjnldsv merged commit ae1b76a into master Apr 12, 2022

delete-merged-branch bot deleted the fix/public-handling branch April 12, 2022 08:56

come-nc reviewed Apr 12, 2022

View reviewed changes

src/public.js

let count = limit - downloads

let clicks = 0

const updateCounter = function(span) {

Copy link

Contributor

come-nc Apr 12, 2022

Choose a reason for hiding this comment

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

missing count parameter no?

come-nc reviewed Apr 12, 2022

View reviewed changes

skjnldsv mentioned this pull request Apr 12, 2022

1.0.0 #85

Merged

Pytal mentioned this pull request May 4, 2022

Prevent video file downloads when there is a download limit nextcloud/files_videoplayer#274

Merged

Fix public handling of limited shares #76

Fix public handling of limited shares #76

Uh oh!

Conversation

skjnldsv commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szaimen commented Apr 8, 2022

Uh oh!

skjnldsv commented Apr 8, 2022

Uh oh!

szaimen commented Apr 8, 2022

Uh oh!

skjnldsv commented Apr 8, 2022

Uh oh!

szaimen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szaimen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

szaimen left a comment

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Apr 12, 2022

Uh oh!

szaimen commented Apr 12, 2022

Uh oh!

szaimen left a comment

Choose a reason for hiding this comment

Uh oh!

szaimen commented Apr 12, 2022

Uh oh!

skjnldsv commented Apr 12, 2022

Uh oh!

szaimen commented Apr 12, 2022

Uh oh!

szaimen left a comment

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Apr 12, 2022

Uh oh!

come-nc Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

come-nc Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skjnldsv commented Apr 8, 2022 •

edited

Loading

szaimen left a comment •

edited

Loading

skjnldsv commented Apr 12, 2022 •

edited

Loading