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

File uploads #320

Merged
merged 6 commits into from
Jul 21, 2015
Merged

File uploads #320

merged 6 commits into from
Jul 21, 2015

Conversation

rockhouse
Copy link
Contributor

Hey guys, this is my first approach to the file upload functionality #24. It still needs work but I thought it would be good to have a discussion before more work is done. Do you like the approach? Can you help me with some open issues:

  1. There should be some animation like a greyed out screen
  2. Permissions need to be adjusted still getting an error (hate allow/deny)

The choice here is CollectionFS (https://github.com/CollectionFS/Meteor-CollectionFS). Issues with that package are: it is not stable and is discouraged by the developer to be used in production, second: it uses allow/deny rules and not methods which I personally don't like. Third: it implements HTTP endpoints which are convenient but also very open I feel.

Besides that, it does what it should. There is a possibility to extend to S3, at the moment it uses as discussed GridFS/MongoDB as storage.

What are your comments, thoughts and suggestions?

@engelgabriel
Copy link
Member

Thanks!! Can you update the fork so I can try to merge again?

Conflicts:
	.meteor/packages
	client/views/app/room.coffee
	client/views/app/room.html
engelgabriel added a commit that referenced this pull request Jul 21, 2015
@engelgabriel engelgabriel merged commit 7318917 into RocketChat:master Jul 21, 2015
@sampaiodiego
Copy link
Member

@rockhouse as pointed by @engelgabriel on #24 (comment), the uploaded file always point to localhost:3000

do you want some help to do a "stylized drop area"? or is it something you are working on?

@inderpreetsingh
Copy link
Contributor

I have had quite an experience in collection FS, would be happy to help.

@rodrigok
Copy link
Member

Nice @inderpreetsingh.

Can you review our implementation of file upload using Collection FS? Maybe you can see errors and improvements to do :)

Thanks

@rockhouse
Copy link
Contributor Author

Thanks @inderpreetsingh! There is actually still an issue with the allow/deny rules maybe you can have a look at that?

Thanks so much!

@inderpreetsingh
Copy link
Contributor

Aah! somehow I missed the follow up comments. I will surely look into those this weekend.

@rodrigok
Copy link
Member

Hei @inderpreetsingh and @rockhouse, we are considering to move file uploading to use this package https://github.com/jalik/jalik-ufs, what do you think about?

CollectionFS is so big, have a lot of bugs and we can't use a progress bar.

@inderpreetsingh
Copy link
Contributor

@rodrigok I don't have experience with jalik-ufs. I am currently looking into it. In first look it seems a good alternative.

Also, is dropping on the page deliberate design decision ? I was working on adding a upload attachment icon in the message bar.

@rodrigok
Copy link
Member

@inderpreetsingh, it is a good alternative, I did the adapter for gridfs some days ago.

We started with D&D and today I did the button in the message bar for upload file. I already did an modal to confirm and preview file before upload.

@inderpreetsingh
Copy link
Contributor

inderpreetsingh commented Aug 18, 2015 via email

@engelgabriel
Copy link
Member

We had a Trello board, and it was even more complicated, and information and status about issues was spread in two systems that do not talk to each other. The best way is to always comment on an issue saying that you want to start working on it and we should mark what we are working on as IN PROGRESS.

Having said that, this may not be the case here, as I can see there was some discussion already on top here. Maybe @sampaiodiego should add @inderpreetsingh to the file-uploading channel on the demo server?

Looks like a real time chat would be more helpful here.

@rockhouse
Copy link
Contributor Author

@rodrigok jalik's approach to fileupload looks very promising I like the clean implementation compared to CollectionFS. I completely agree with you, it is way to bloated. What is your experience with it until now after your implementation of the adapter? What else needs to be done, maybe I can help at some point?
@engelgabriel / @inderpreetsingh I would also vote for keeping it in github seems the best central point to keep the discussion

@engelgabriel
Copy link
Member

@rockhouse, I agree, lets keep the conversation here.

@ai10
Copy link

ai10 commented Aug 19, 2015

I would love to see a rocketchat-amazons3 package; but am focused on other priorities.

@engelgabriel
Copy link
Member

AWS should be a top priority IMHO.. especially with our big 40k users deployment coming next month. I think we will need it to be able to handle that kind of load.

@rodrigok
Copy link
Member

@rockhouse I have no experience with jalik's packages, but I really can't solve certain problems with CollectionFS because their huge size.

I think jalik-ufs much more accessible to our necessities, and is very easy to create new adapters, I can create an adapter for S3 with no problems in, maybe, 1 day.

If @RocketChat/owners agree, I can start migration today or tomorrow to see how big is this change.

@engelgabriel
Copy link
Member

OK, lets leave it for next week?

@rodrigok
Copy link
Member

Is better launch the 1.0 version with the correct file system, do not you think @engelgabriel?

@engelgabriel
Copy link
Member

True... :(

@rodrigok
Copy link
Member

@rockhouse and @inderpreetsingh, my WIP for this migration #518

@rockhouse
Copy link
Contributor Author

@rodrigok looks awesome! let me check it out in more detail next week. Well done! thanks

@jhnvz
Copy link

jhnvz commented Oct 7, 2015

+1 for an s3 adapter!

@batuhan
Copy link

batuhan commented Nov 14, 2015

Is there any development with the S3 adaptor?

@rodrigok
Copy link
Member

@batuhan not yet.

Peym4n pushed a commit to redlink-gmbh/Rocket.Chat that referenced this pull request Apr 26, 2018
* Implement a local password policy

* Fix Smarti integration tests

* Correct merge-error from https://github.com/RocketChat/Rocket.Chat/pull/9367/files

* Add setting to configure i18n for Smarti widget (RocketChat#308)

* Add setting to configure i18n for Smarti widget

* add exhausting description for the translation object

* Propagate topic value of the expertise (RocketChat#317)

* Help Request creation using word cloud (RocketChat#169)

* enable drop down and fix ac after selection from word cloud
* restrict and sort drop down values to 10
* added icon for wc and moved db call to server methods
* Delay the expertise drop down a bit

* Add some translations

* bump version
HappyTobi pushed a commit to HappyTobi/Rocket.Chat that referenced this pull request Jul 10, 2018
…fication-reply

[NEW] Allow reply notifications on Mac OS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants