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

Snippet message #4623

Merged
merged 24 commits into from Nov 2, 2016
Merged

Snippet message #4623

merged 24 commits into from Nov 2, 2016

Conversation

@juanwolf
Copy link
Contributor

@juanwolf juanwolf commented Oct 13, 2016

@RocketChat/core

Closes #4395 [Closes #1553 but no auto-recognition]

I made a plugin to handle snippets into RocketChat. It is not finished yet but some review from the community would be awesome.

How it works

  • Send your code as a normal message
  • Use the toolbox on your message such as below

snippet-plugin-message-toolbox

- Choose a name for your snippet

snippet-plugin-alert-msg

- Confirmation Message

snippet-plugin-confirmation-alert

- Snippet created!

After that, the message containing the snippet is formatted as you could do it using the "```". The system also create a new message containing the link to access to your snippet.

message-snippet-plugin-after-party

You can also access to your snippet with the tab bar on your right.

screen shot 2016-10-13 at 13 49 30

- View your snippet

snippet-plugin-view

# Settings ## Message configuration

You can disable or enable this feature in the "Message" administration section

screen shot 2016-10-13 at 15 28 18

## ~~Snippet storage configuration~~

You have the choice between GridFS storage and FileSystem storage

screen shot 2016-10-13 at 15 14 09

Please do not hesitate to review the code and give me your thoughts about this plugin :)

Edit 14/10

  1. The persistence of the message as a file has been removed because it was not really useful.
  2. Snippets are now stored only into the rocketchat_messages collection instead of having a rocketchat_snippeted_message
  3. The snippet is downloadable
@cpitman
Copy link
Contributor

@cpitman cpitman commented Oct 14, 2016

Do snippets really need to be stored separately? Could it just be a property of a normal message?

@juanwolf
Copy link
Contributor Author

@juanwolf juanwolf commented Oct 14, 2016

No not really. I based this plugin on the rocketchat-pin-message which does save messages in a different collections. There's already a property in the rocketchat-message collection for a snippet message so it should be quick to do the changes.

Jean-Loup Adde added 2 commits Oct 14, 2016
@cpitman
Copy link
Contributor

@cpitman cpitman commented Oct 14, 2016

Either way, this will help a ton with users that are pasting in large chunks of code, console output, etc.

@sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented Oct 14, 2016

nice work @juanwolf .. =)

snippet messages are still limited by message size limit, right?

@juanwolf
Copy link
Contributor Author

@juanwolf juanwolf commented Oct 14, 2016

@sampaiodiego thanks! Yes. The snippet message is stored as a normal message so the limitation should still work.

@engelgabriel engelgabriel added this to the 0.44.0 milestone Oct 17, 2016
validation: function(message) {
let room = RocketChat.models.Rooms.findOne({_id: message.rid});

if (Array.isArray(room.usernames) && (room.usernames.indexOf(Meteor.user().username) === -1)) {

This comment has been minimized.

return false;
} else {
if (message.snippeted || ((RocketChat.settings.get('Message_AllowSnippeting') === undefined) ||
(RocketChat.settings.get('Message_AllowSnippeting') === null))) {

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

Message_AllowSnippeting can be false, right?

This comment has been minimized.

@juanwolf

juanwolf Oct 25, 2016
Author Contributor

Yeah right, focused on the null value but not the proper one 😞

</div>
</div>

</template>

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

Can you fix indentation here? Please, do not mix spaces and tabs (we prefer tabs)

let message = Messages.findOne({ _id: FlowRouter.getParam('snippetId') });
if (message === undefined) {
return null;
} else {

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

The else block is not needed here, the return will stop the code execution 😄


Template.snippetPage.onCreated(function() {
let snippetId = FlowRouter.getParam('snippetId');
console.log(`${snippetId}: ${this.snippet}`);

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

Can you remove this log?

{ '_id': _id }
).observeChanges({
added: function(_id, record) {
publication.added('rocketchat_message', _id, record);

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

We should not use the real collection name here, can you replace by rocketchat_snippeted_message

@@ -0,0 +1 @@
this.Messages = new Meteor.Collection('rocketchat_message');

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

We should not use the real collection name here, can you replace by rocketchat_snippeted_message

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

And, can you change the variable name? This variable will store only messages from snippeted messages and only when you have the tab bar open

This comment has been minimized.

@juanwolf

juanwolf Oct 25, 2016
Author Contributor

I will do that! Why should we not use the 'rocketchat_message' collection?

}
).observeChanges({
added: function(_id, record) {
publication.added('rocketchat_message', _id, record);

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

We should not use the real collection name here, can you replace by rocketchat_snippeted_message

var match = /^\/([^\/]+)\/(.*)/.exec(req.url);

if (match[1]) {
let snippet = RocketChat.models.Messages.findOneById(match[1]);

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

You should do this find after login verification, cuz it's not necessary if user is not authorized.

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

Can you verify if the message is a snippet, otherwise users can download any message

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

And, can you verify if the user has access to the room where this message came from?

@@ -0,0 +1,13 @@
Meteor.startup(function() {
RocketChat.settings.add('Message_AllowSnippeting', true, {

This comment has been minimized.

@rodrigok

rodrigok Oct 24, 2016
Member

We usually start new features as disabled by default then users can enable and test, can you change to false?

@juanwolf
Copy link
Contributor Author

@juanwolf juanwolf commented Oct 25, 2016

Thanks for the code review @rodrigok ! I did some modifications you asked for, I will continue to work on it tomorrow.

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Oct 25, 2016

@juanwolf awesome, thanks

@rodrigok rodrigok modified the milestones: 0.44.0, 0.45.0 Oct 25, 2016
@juanwolf
Copy link
Contributor Author

@juanwolf juanwolf commented Oct 26, 2016

@rodrigok Little question. Because you mentioned that permissions were not set in the request.js file, I wanted to manage it to directly in the snippet page as well. Unfortunately it's just front-end (using FlowRouter) so I can't do proper queries to the mongo, I managed permissions into the publication 'snippetedMessage' but it gives me an empty template (which makes sense but that's really ugly).

I was wondering if you have an idea of where I could manage this permission and send to the user a proper 403 response.

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Oct 26, 2016

@juanwolf We can start with this ugly solution and start a new PR for improvements. One way is use method to get data instead of subscriptions, in fact we are replacing a lot of subscriptions by methods, then you can receive the response for your single record or an error and manage the template to show the correct result easily.

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Oct 27, 2016

@juanwolf Can you fix the conflict?

@juanwolf
Copy link
Contributor Author

@juanwolf juanwolf commented Oct 27, 2016

@rodrigok I am on it !

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Oct 31, 2016

@engelgabriel IMHO we should merge this in release 0.46

@engelgabriel engelgabriel modified the milestones: 0.45.0, 0.46.0 Nov 1, 2016
@engelgabriel engelgabriel merged commit 2f8c637 into RocketChat:develop Nov 2, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@B0biN
Copy link

@B0biN B0biN commented May 18, 2017

Required to allow in permissions for normal users!!!

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

Successfully merging this pull request may close these issues.

6 participants