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

Add uploading support for registered users #574

Merged
merged 5 commits into from
Mar 11, 2016
Merged

Conversation

oparoz
Copy link
Contributor

@oparoz oparoz commented Mar 9, 2016

Fixes #25. It replaces #485 and #520 .

Featuring

  • Drag and drop component
  • Folder uploading on Chrome
  • File comparison dialogue
  • [ + ] button
  • Improved empty page, now with error messages

Todo

Test plan

Base

  • Uploading file(s) via drag and drop
  • Uploading a folder via drag and drop
  • Uploading file(s) via the [+] menu

Additional tests

  • Uploading from an empty Gallery should work and the UI should adapt to the content of the album
  • Uploading the same file twice should bring up the conflict dialogue

Tech review

  • The javascript used in files is loaded as-is. A special file has been created to replace FileList and Files methods called by those scripts
  • newfilemenu.js has been imported and modified to make it work with Gallery. We don't need more than the upload entry
  • OC.Upload._isReceivedSharedFile is replaced with a method which does the lookup in the cache instead of in the HTML
  • Some methods were moved from Gallery to GalleryView as it made more sense to have them there
  • GalleryImage has 2 new properties: size and sharedWithUser
  • The landing page for empty albums has been modified to take into consideration the context in which that page is called and can print the error message, if communicated to the frontend

Note: Ideally, we should switch to webdav once uploading is supported and re-usable components for the file dialogue move outside of files.


@karlitschek @jancborchardt @DeepDiver1975 @LukasReschke @PVince81 @MorrisJobke @jospoortvliet @setnes @deMattin @arkascha @elpraga @sualko @Bugsbane @mmattel @tahaalibra @viraj96 @imjalpreet @mbtamuli @rahulgoyal030

@oparoz oparoz self-assigned this Mar 9, 2016
@oparoz oparoz added this to the 9.1-current milestone Mar 9, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @jancborchardt and @PVince81 to be potential reviewers

@tahaalibra
Copy link
Contributor

Working

  • Uploading file(s) via drag and drop
  • Uploading a folder via drag and drop
  • Uploading file(s) via the [+] menu
  • Uploading from an empty Gallery
  • UI is adapting to the changes
  • Uploading Same files gives the warning

great 👍 ...everything seems to be working fine

I will try some more test and take a look at the code..

@imjalpreet
Copy link
Member

@oparoz Everything seems to be working! 👍
Except a few points that I will be mentioning at the end.
I have tested the following:

  • Drag and Drop Upload for both file(s) and folder.
  • Upload file(s) via [+] button.
  • Uploading from an empty Gallery is working (and the UI adapts to the images and also to window resize)
  • Conflict Dialogue comes if same files are uploaded more than once.
  • Error Messages

But I wanted to clarify one thing, if we try to upload a folder and even if there is one empty file, the entire folder doesn't get uploaded. I think it should not be like this. It think we should try to atleast upload the non empty files and folders.

Secondly, the error message that is displayed in the above case should be changed. Currently, the error message is 'Error uploading file "Untitled Document": Unable to upload Untitled Document as it is a directory or has 0 bytes'. Since, directory upload is now possible, we should not print in the error message that the reason of not uploading is that it is a directory. I am attaching a screenshot of the following.

One more thing we can do is automatically hide the error message after a certain time instead of keeping it till someone clicks on 'x'.
errormessage

@tahaalibra
Copy link
Contributor

I am getting javascript error when viewing a public gallery..it is preventing gallery top open
''galleryview.js?v=95a8dd6…:276 Uncaught TypeError: Cannot set property '_isReceivedSharedFile' of undefined"
selection_001

@oparoz

@oparoz oparoz force-pushed the official-upload-support branch from a16e4ff to 6760153 Compare March 10, 2016 11:11
@imjalpreet
Copy link
Member

@oparoz Now public gallery is also working fine! 👍

@oparoz
Copy link
Contributor Author

oparoz commented Mar 10, 2016

I am getting javascript error when viewing a public gallery..it is preventing gallery top open
''galleryview.js?v=95a8dd6…:276 Uncaught TypeError: Cannot set property '_isReceivedSharedFile' of undefined"

@tahaalibra - Ah, yes. I've changed approach at the 11th hour and failed to verify that it still worked on the public side. That side doesn't support uploading yet. It's now fixed.

@imjalpreet - All the problems you've described should happen on the Files side as well and some reports have probably already been opened in core. Go see if that's the case and if not, add your suggestions. Some of these issues are not trivial to fix, but if you have solutions, core is also accepting PRs :)

@imjalpreet
Copy link
Member

@oparoz Yeah Sure. I will have a look in core.

@tahaalibra
Copy link
Contributor

@oparoz now working on public as well as logged in !!!!!

@oparoz oparoz force-pushed the official-upload-support branch from 6760153 to dfcb16d Compare March 11, 2016 18:26
oparoz added a commit that referenced this pull request Mar 11, 2016
Add uploading support for registered users
@oparoz oparoz merged commit 65d5514 into master Mar 11, 2016
@oparoz oparoz deleted the official-upload-support branch March 11, 2016 18:41
@jospoortvliet
Copy link

Awesome work @oparoz !

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

Successfully merging this pull request may close these issues.

5 participants