Skip to content
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

Upload to multiple locations (new Attempt) #73

Merged
merged 66 commits into from
May 23, 2020

Conversation

JonathanTreffler
Copy link
Collaborator

@JonathanTreffler JonathanTreffler commented Nov 17, 2019

new attempt to implement this PR
Reusing some code from NastuzziSamy (not anymore because we moved no vue.js)
I am not forking this directly from NastuzziSamy because so much changed in flowupload master.

@JonathanTreffler
Copy link
Collaborator Author

Theres a lot to do because especially app.js and the different controllers aren't compatible to Flowupload master

@JonathanTreffler
Copy link
Collaborator Author

JonathanTreffler commented Nov 17, 2019

TODO:

  • integrating features from NastuzziSamy
  • reorganise angular controllers
  • Moving "Add File" and "add Folder buttons out of sidebar
  • Drag and Drop
  • Moving Locations to client side
  • starred and unstarred locations frontend
  • starred and unstarred locations backend
  • Drag and Drop folder fix
  • remove ng-flow
  • Location deletion
  • better permission checks before chunk assembling
  • upload.php code cleanup
  • App.vue code cleanup
  • Database connection (for starred locations)
  • new README GIF (?)
  • Security testing
  • Dark Mode testing
  • testing on Nextcloud 16
  • testing on Nextcloud 17
  • testing on Nextcloud 18
  • testing on Nextcloud 19
  • fix build script for Webpack/NPM
  • Add warning that not all external storages are supported

@JonathanTreffler
Copy link
Collaborator Author

I integrated everything from NastuzziSamy into the current version of flowupload.
But i think there are many improvements to do:

I don't get why adding files and folders should be in the sidebar. (Is it possible to implement them like they were before with multiple upload folders ?)

There are a few other UI changes i would like to make.

And another question is: Why are the locations saved server side ? Wouldn't it be enough to handle them client side ?

@e-alfred
Copy link
Owner

e-alfred commented Nov 20, 2019

I don't get why adding files and folders should be in the sidebar. (Is it possible to implement them like they were before with multiple upload folders ?)

I am not sure either, the UI design with the old PR is somewhat problematic and we should use the official folder selection UI and other components to make it more coherent with the rest of Nextcloud anyway. Do you have a screenshot/list of changes to the UI you want to make?

Why are the locations saved server side ? Wouldn't it be enough to handle them client side ?

Probably yes, but it might be convenient for users to reuse locations used in the past.

@JonathanTreffler
Copy link
Collaborator Author

JonathanTreffler commented Nov 20, 2019

I changed the add location design:

before:

image
clicked:
image

now:

image
clicked:
image

Now its more in line with the nextcloud design (see here)

@JonathanTreffler
Copy link
Collaborator Author

Do you have a screenshot/list of changes to the UI you want to make?

I will add them to the TODO List

@JonathanTreffler
Copy link
Collaborator Author

the UI design with the old PR is somewhat problematic

I agree

@JonathanTreffler
Copy link
Collaborator Author

Probably yes, but it might be convenient for users to reuse locations used in the past.

My Browser saves the previous paths as autofills , but we could implement account wide autofill reccomendations.
image

I personally wouldn't like having maybe unneccesary destinations in that list on every client.
Also server side would really mess up multiple instances of flowupload on the same account.

@JonathanTreffler
Copy link
Collaborator Author

folder selection UI

What do you mean by that ?

This ?
image

or something different ?

@JonathanTreffler
Copy link
Collaborator Author

This is how it looks now:

image

@JonathanTreffler
Copy link
Collaborator Author

This is how it looks now:

image

I had to completely replicate the behaviour of the two buttons because the ng-flow implementation isn't flexible enough. And now the buttons work.

@e-alfred
Copy link
Owner

e-alfred commented Nov 21, 2019

Nice work! About the folder picker I thought about this one (used in the Collabora app):

image

The code can be found here:

https://github.com/nextcloud/richdocuments/blob/5c306b1244ead1e80d35b2deac3bfe4a02237330/src/personal.js#L10

https://github.com/nextcloud/richdocuments/blob/e73d3a4d9ede9d8167056f0f77d65334b27737de/templates/personal.php#L12

@JonathanTreffler
Copy link
Collaborator Author

About the folder picker I thought about this one (used in the Collabora app):

That looks really nice. So how should we do the UI ?

@JonathanTreffler
Copy link
Collaborator Author

Do we want to keep this ?
image
or completely use the Folder Picker dialog ?

@e-alfred
Copy link
Owner

I tested you zipped version, it works pretty well. UI wise, do we need the settings menu in the bottom left corner at the moment? It is pretty empty. Maybe this feature could be set there in the future: #75

@e-alfred
Copy link
Owner

Seems like I was a bit too fast assuming everything works. I tried to upload to an external storage and it immediately shows "error" after finishing the upload. The file is never created in the folder either showing these messages:

$ is deprecated: The global jQuery is deprecated. It will be updated to v2.4 in Nextcloud 20 and v3.x in Nextcloud 21. In later versions of Nextcloud it might be removed completely. Please ship your own. 2 globals.js:61:15
Object { flowObj: Getter & Setter, bytes: Getter & Setter, file: Getter & Setter, name: Getter & Setter, size: Getter & Setter, relativePath: Getter & Setter, uniqueIdentifier: Getter & Setter, chunkSize: Getter & Setter, chunks: Getter & Setter, paused: Getter & Setter, … }
App.vue:465
[Vue warn]: Duplicate keys detected: 'false'. This may cause an update error.

found in

---> <App> at src/App.vue
       <Root> vue.runtime.esm.js:619
[Vue warn]: Duplicate keys detected: 'false'. This may cause an update error.

found in

---> <App> at src/App.vue
       <Root> 2 vue.runtime.esm.js:619

@JonathanTreffler
Copy link
Collaborator Author

Seems like I was a bit too fast assuming everything works. I tried to upload to an external storage and it immediately shows "error" after finishing the upload. The file is never created in the folder either showing these messages:

Well i kind of knew that. Many external storages don't implement the functions we need to assemble the chunks. We need a fallback method for assembling the chunks, but that will not be easy. I will try it in the next few days, but i can't guarantee that it will work with every External Storage.

@JonathanTreffler
Copy link
Collaborator Author

I tested you zipped version, it works pretty well. UI wise, do we need the settings menu in the bottom left corner at the moment? It is pretty empty. Maybe this feature could be set there in the future: #75

#75 is exactly the reason why it's there, but we can comment it out for the release.

@e-alfred
Copy link
Owner

Many external storages don't implement the functions we need to assemble the chunks.

Well, if that is the case, then maybe whitelisting might be an option. Do you know which ones work? I tested it with SFTP which seems not to work correctly at the moment.

@JonathanTreffler
Copy link
Collaborator Author

Well, if that is the case, then maybe whitelisting might be an option. Do you know which ones work? I tested it with SFTP which seems not to work correctly at the moment.

Everyone that implements the fopen function should work. But that's not many so we need to find another way of assembling the chunks, if fopen isn't implemented. fopen is very efficient so we should use it whenever we can

@e-alfred
Copy link
Owner

Everyone that implements the fopen function should work.

We should at least put a warning somewhere for now, otherwise people will open issues again about this even if we know that not all external storage types will work at the moment. Whitelisting would still be a better idea to limit functionality to working storage types.

@e-alfred
Copy link
Owner

Do you think we can merge it already? The PR still has the "draft" status.

@JonathanTreffler
Copy link
Collaborator Author

I updated the README to make it very clear that External Storages may not work yet.
I think we can merge this very soon. I will briefly go over the source code to see if i can see anything that still needs to be done.
I didn't test this PR on Nextcloud 16 and 17 but i am sure it will work. I don't think we use any new APIs that could break this app on old Nextcloud Versions.

@JonathanTreffler JonathanTreffler marked this pull request as ready for review May 23, 2020 11:43
@JonathanTreffler
Copy link
Collaborator Author

I added Strings to the Project that will need to be translated, but Transifex will take care of that. We don't have to exclude /js/ with l10nignore because no webpack built code will be in the repository (only in releases).

@JonathanTreffler
Copy link
Collaborator Author

I didn't spot any problems in the code. Are we ready to merge ?

@JonathanTreffler
Copy link
Collaborator Author

Codacity doesn't like my code (mostly because mixed " and '. And some of these warnings are totally useless) but i will clean that up later. Our priority right now should be supporting Nextcloud 19, wich will be released very soon.

@e-alfred e-alfred self-requested a review May 23, 2020 12:50
@e-alfred
Copy link
Owner

e-alfred commented May 23, 2020

I am not sure if Codacy is that useful for this project at all, but in the future Dependabot is useful probably for keeping our dependencies current.

We could make a release for Nextcloud 19 and 18 only for now so older releases can still use the older legacy version of Flowupload that works still on those Nextcloud releases. Translations will be done by the awesome translators at Transiflex so they should be pushed here within a few days, if some strings are left that could be solved by a minor release.

The only thing left is the release script in the .ci folder, I am not sure if it is needed anymore.

@JonathanTreffler
Copy link
Collaborator Author

The only thing left is the release script in the .ci folder, I am not sure if it is needed anymore.

You could do the release manually or we have to update the script to run webpack before the release

@e-alfred e-alfred merged commit d88057f into e-alfred:master May 23, 2020
@e-alfred
Copy link
Owner

Its done! I also enabled Dependabot because we already have a security vulnerability in our dependencies (minimist).

@e-alfred e-alfred linked an issue May 23, 2020 that may be closed by this pull request
@JonathanTreffler JonathanTreffler deleted the multiple-locations branch May 23, 2020 18:07
@e-alfred
Copy link
Owner

Flowupload 1.0 release is done: https://apps.nextcloud.com/apps/flowupload

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants