Skip to content
This repository has been archived by the owner on Jun 5, 2019. It is now read-only.

Activity Stream #79

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Activity Stream #79

wants to merge 11 commits into from

Conversation

steffen
Copy link

@steffen steffen commented Dec 12, 2014

Branch for activity stream commits. Still WIP. Will post comments in https://assembly.com/buckets/bounties/82 to keep it all in one thread.

type = escapeExpression resource.type
name = escapeExpression resource.name
email = escapeExpression resource.email
bucketSlug = escapeExpression resource.bucket?.slug
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all the escaping necessary? Since we're supplying the resource ourself, and it goes through SafeString below-

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't like what I was doing here (or what I had to do here) either. In general I was hoping to have some link and url helpers available to do something like: link_to admin_user_path user. More on that in response to your other comment.
Regarding the escaping and using SafeString, I understood that because I'm using SafeString, I need to do the escaping (since SafeString expects a safe string).
See the link helper example on http://handlebarsjs.com/, where it says:

Handlebars will not escape a Handlebars.SafeString. If you write a helper that generates its own HTML, you will usually want to return a new Handlebars.SafeString(result). In such a circumstance, you will want to manually escape parameters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sorry, you're right. In that case, though, let's just escape name since I think it's the only one susceptible to invalid user input-

@steffen
Copy link
Author

steffen commented Jan 22, 2015

Merged the refs version into this one and adjusted the tests. There are still some more pending tests to be implemented which I may get to in the next days.

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.

2 participants