Skip to content

Conversation

@TSI-yogeshshejwadkar
Copy link

@TSI-yogeshshejwadkar TSI-yogeshshejwadkar commented Mar 21, 2022

  1. Fixed the issue to accept 0 and null
    Use case for null value - If I remove existing value from the text box, the download limit should get removed for that particular share.
    Use case for 0 value - If I put 0 or change the existing value to 0, then the download limit should get removed for that particular share.
  2. Changed the default text value from total download count to remaining download count
  3. Changed the hover text from Download limit: to Downloads:
  4. Changed the implementation to allow to set the download limit to the file having allow resharing permissions

…ue to remaining downloads

Signed-off-by: Yogesh Shejwadkar <[email protected]>
@TSI-yogeshshejwadkar TSI-yogeshshejwadkar force-pushed the nmcfeat/1042-download-limit-adjustments branch from e32161f to 1bf86a3 Compare March 22, 2022 04:53
@juliusknorr juliusknorr requested a review from skjnldsv March 22, 2022 06:34
@skjnldsv skjnldsv requested review from a team, come-nc and vanpertsch and removed request for a team March 22, 2022 16:49
@skjnldsv skjnldsv added the bug Something isn't working label Mar 22, 2022
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Why is null needed? The same thing seems to be done for 0 or null, so it would be better if null was turned into 0 before calling the method.

title: t('files_downloadlimit', 'Download count: {count}', this._store),
value: this._store.limit,
title: t('files_downloadlimit', 'Downloads: {count}', this._store),
value: this._store.limit ? this._store.limit - this._store.count : this._store.limit,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this change makes sense, the label says download limit so it should show the limit, not something else.

Choose a reason for hiding this comment

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

It makes sense because the remaining downloads is the current download limit. To show the initial download limit one week later to the customer does not deliver any new information to the user because he may remember the initial download limit or can calculate it with the download counter. It is more helpful to tell the user the current download limit to enable him to decide if he wants to increase it or not.

@come-nc
Copy link
Contributor

come-nc commented Apr 4, 2022

Please run composer run cs:fix
And I am still not sold on the label text change. @jancborchardt any opinion on #69 (comment) ?

@jancborchardt
Copy link
Member

@come-nc @TSI-yogeshshejwadkar what is the change – can I get before/after screenshots? Thanks! :)

@TSI-yogeshshejwadkar
Copy link
Author

TSI-yogeshshejwadkar commented Apr 6, 2022

@come-nc @TSI-yogeshshejwadkar what is the change – can I get before/after screenshots? Thanks! :)

@jancborchardt
limit2
Here I set download limit to 5.

limit1
Here both the counters changed, the text is showing 4 (remaining count) and hover label is 1 (downloaded count) as I downloaded the file once.
Previously here the text was showing 5 (total count), and hover label was showing 1 (downloaded count) and this is the change.

…file having allow resharing permissions

Signed-off-by: Yogesh Shejwadkar <[email protected]>
@TSI-yogeshshejwadkar
Copy link
Author

@come-nc @skjnldsv @jancborchardt @juliushaertl
Any update on this? Is it good to merge?

@come-nc
Copy link
Contributor

come-nc commented Apr 12, 2022

If #77 is ruled as a yes then I am for merging this.
But there are conflicts with master.

@skjnldsv
Copy link
Member

Please wait for #76 first too.
Also, there are changes we cannot accept here, let me add comments

@skjnldsv
Copy link
Member

skjnldsv commented Apr 12, 2022

4. Changed the implementation to allow to set the download limit to the file having allow resharing permissions

Already handled by #79
You changes is unsecure and allows anyone

@skjnldsv skjnldsv closed this in #82 Apr 12, 2022
@skjnldsv skjnldsv mentioned this pull request Apr 12, 2022
@tsdicloud tsdicloud deleted the nmcfeat/1042-download-limit-adjustments branch July 13, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants