Skip to content
This repository was archived by the owner on May 22, 2021. It is now read-only.

Show Download Limits on upload page: Solves #661 #681

Closed
wants to merge 33 commits into from

Conversation

shikhar-scs
Copy link
Contributor

@shikhar-scs shikhar-scs commented Dec 21, 2017

As suggested by @SoftVision-CiprianMuresan at #661, the downloads available can now be seen on the main upload page. This is the basic work done.

Preview

screenshot from 2017-12-21 18-37-12

Due to non-availability of translated texts, I could not change the table headers as to my convenience and so the Expires in column is repeated. Also an svg image icon can be used instead of the word d'load (complete downloads was not fitting in the table cell) However the download limits can now be seen on the main page.

Please review @sevaan @dannycoates @SoftVision-CiprianMuresan

@himanish-star
Copy link
Contributor

@shikhar-scs ,cool

@shikhar-scs
Copy link
Contributor Author

@sevaan
I've edited the column headers as reuired (even though something better would be if we could individually put ${state.translate(<table header name>)} but that wont be much of a task I guess once we have the required statements in all the languages.
Also, the width of file name column has been improved considerably as required by you. Here's a preview.

screenshot from 2017-12-22 09-34-38

Please review

@shikhar-scs
Copy link
Contributor Author

@dannycoates mind giving a review??

@sevaan
Copy link

sevaan commented Dec 22, 2017

Looks great, @shikhar-scs. Thanks!

@shikhar-scs
Copy link
Contributor Author

shikhar-scs commented Dec 23, 2017

@sevaan welcome. 😀
Then is it a merge ??

@sevaan
Copy link

sevaan commented Dec 23, 2017

I don't do code reviews, so wait until @dannycoates gives it a look over. But from a front-end visual POV, looks good.

@shikhar-scs
Copy link
Contributor Author

shikhar-scs commented Dec 23, 2017

@sevaan ok, thanx. I'll surely wait for @dannycoates

@shikhar-scs
Copy link
Contributor Author

@dannycoates mind reviewing here please ?

@dannycoates
Copy link
Contributor

This is going to need more work, including server api changes, in order to stay updated between page refreshes and as the download count changes. I think it'd be better if I take on this task. I'm sure @shikhar-scs could do it, but there's some other aspects of the design I'd like to change that also affect this, and it'd probably take us just as long to talk about it as it would for me to just do it.

@dannycoates dannycoates closed this Jan 5, 2018
@shikhar-scs
Copy link
Contributor Author

@dannycoates

and it'd probably take us just as long to talk about it as it would for me to just do it.

Even if that's the case, still I would like to work on this as I had already invested much time and thus, this PR going away like this is not good for me.

With time I've only understood more and more of the code base.

And I believe with a little bit of guidance I'll be able to do all of the stuff you require. If you could just make one long comment as to where the rest of the changes have to be made, I'll see to my self that they work. And if they dont I'll be more than happy to let you take over.

I'm sure @shikhar-scs could do it, but there's some other aspects of the design I'd like to change

Once I'm done with the backend stuff you can incorporate all the design changes.(or mention it in the comment itself and I'll do that ) Rest is up to you.

@shikhar-scs
Copy link
Contributor Author

@dannycoates any thoughts on this ? 🤞

@dannycoates
Copy link
Contributor

Ok @shikhar-scs. If you're in this for the long haul I'll help guide you through it. You can expect this to be a pretty big effort and will likely take a fair amount of time. I will have some time later this week to come up with a plan. Stay tuned.

@shikhar-scs
Copy link
Contributor Author

yep Sure I am ready to invest time if the need be and also tell me if I need to remind you about the same 😄

@dannycoates
Copy link
Contributor

Ok, @shikhar-scs, here we go...

In app/fileManager.js there's a function named checkFiles. It currently calls exists which makes a request to the server to see if it still exists. We'll want to instead call a new function that makes a request to /api/metadata in order to get the download counts. Fortunately I think we can use the getMetadata method of FileReceiver. We'll also probably need to change the rerender logic a bit too.

There's a setInterval at the bottom of fileManager.js that rerenders periodically to keep the expire time up to date. Now we also want to keep the counts up to date so we should change that render call to checkFiles.

The rest should just be updating the UI parts, which you've already started on. In the downloads column I think we should show both the number of downloads that happened dtotal and the limit dlimit as (psuedocode) "${dtotal} / ${dlimit}" ex. "2 / 5"

@shikhar-scs
Copy link
Contributor Author

Awesome !!! I'll get started with it right away. Thank you.

@himanish-star
Copy link
Contributor

@shikhar-scs , awesome

@shikhar-scs
Copy link
Contributor Author

@dannycoates I'll tell you my approach.

It currently calls exists which makes a request to the server to see if it still exists.

Using the else part of if(!ok), I wrote my checkDL method to receive metadata respective to that file. This ensured that we are asking metadata of files which actually exist.

We'll want to instead call a new function that makes a request to /api/metadata in order to get the download counts. Fortunately I think we can use the getMetadata method of FileReceiver.

In the new method, taking cue from this following snippet on how to use the FIleReceiver methods,

send/app/fileManager.js

Lines 188 to 196 in bace117

emitter.on('preview', async () => {
const file = state.fileInfo;
const url = `/api/download/${file.id}`;
const receiver = new FileReceiver(url, file);
receiver.on('progress', updateProgress);
receiver.on('decrypting', render);
state.transfer = receiver;
try {
await receiver.getMetadata(file.nonce);

I wrote a similar method in order to use the getMetadata method of FileReceiver.

const url = `/api/metadata/${file.id}`;
const receiver = new FileReceiver(url, file);

However, the file which is sent in case of the on('preview' example to FileReceiver is taken from state.fileinfo (see permalink above ). But, the file which is used in my method is taken from state.storage.files.

const files = state.storage.files;

The difference is that files taken from state.storage.files don't have the following or similar attributes namely file.key , secretKeyPromise, authKeyPromise, or metaKeyPromise. Because of this, when, b64ToArray(file.key), method is called on the new file.key, it returns a null error. And in general, every constructor function returns errors in the console. But, the ones from state.fileInfo do contain them and thus, this works smoothly in that case.

I also tried using then state.fileInfo but, I guess it is a specific file related option, something not suitable to be used in case of checkFiles() where we operate collectively, on all the files.

There's a setInterval at the bottom of fileManager.js that rerenders periodically to keep the expire time up to date. Now we also want to keep the counts up to date so we should change that render call to checkFiles.
The rest should just be updating the UI parts, which you've already started on. In the downloads column I think we should show both the number of downloads that happened dtotal and the limit dlimit as (psuedocode) "${dtotal} / ${dlimit}" ex. "2 / 5"

This however looks rather simpler comparatively to manage.

Could you please guide on how to tackle the above problem.

@dannycoates
Copy link
Contributor

Because of this, when, b64ToArray(file.key), method is called on the new file.key, it returns a null error.

I think you're close but I have a feeling you aren't up to date with the master branch.

@shikhar-scs
Copy link
Contributor Author

I think you're close but I have a feeling you aren't up to date with the master branch.

True that, life saviour 😄 .

@shikhar-scs
Copy link
Contributor Author

shikhar-scs commented Jan 14, 2018

Ok @dannycoates, I've implemented changes and please have a look. I've pushed them at shikhar-scs@662e185

I guess dynamic changes are still not occurring. They do occur on refresh though. Here's a preview.

dlimiitt

The main difference between dynamic display of time and downloads is that file.createdAt is a const value. Thus, whenever the time dynamically changes, it is not fetched from the backend. It happens on the frontend only, based on Date.now() - file.createdAt().

However, in case of downloads display, it has to be fetched from the backend (as far as my knowledge goes, I may be wrong though) and thus it needs to be refreshed everytime for displaying the same.

Have a look at the preview for more details.

P.S. If you could re-open this PR please

@shikhar-scs
Copy link
Contributor Author

@dannycoates mind reviewing?

@dannycoates dannycoates reopened this Jan 16, 2018
@dannycoates
Copy link
Contributor

mind reviewing?

yep. please push your changes to this PR's branch.

@shikhar-scs
Copy link
Contributor Author

I'll push them in a while, but is the content of the gif file good

@shikhar-scs
Copy link
Contributor Author

@dannycoates I had already pushed my commits when the PR was in a closed state. Also, the changes do reflect here. Though it isn't showing a new commit. Please go to file diff to see the changes.

Also the CI tests fail because of some similar reasons.

Thomas Dalichow and others added 26 commits January 23, 2018 07:16
Localization authors:
- Jim Spentzos <jamesspentzos@hotmail.com>
Localization authors:
- Juan Esteban Ajsivinac Sián <ajtzibsyan@yahoo.com>
…Send

Localization authors:
- Emin Mastizada <emin@mastizada.com>
Localization authors:
- eljuno <eljunotrie_anggoro@yahoo.co.id>
Localization authors:
- eljuno <eljunotrie_anggoro@yahoo.co.id>
Localization authors:
- Khaled Hosny <khaledhosny@eglug.org>
Localization authors:
- Kerim Kalamujić <kerim@mozilla.ba>
improved font family

font changes
Changed * to ●
included global MAXFILESIZE
Localization authors:
- Jordi Serratosa <jordis@softcatala.cat>
Localization authors:
- Jordi Serratosa <jordis@softcatala.cat>
@shikhar-scs
Copy link
Contributor Author

@dannycoates due to a poor rebase earlier, it led to a bad squashing result. This got out of control. So i've filed a new PR at #721. Please have a look there. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.