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

Activity events / notifications #2174

Merged
merged 18 commits into from
Jul 8, 2016
Merged

Activity events / notifications #2174

merged 18 commits into from
Jul 8, 2016

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Jul 4, 2016

When an app reports activity in a grain that you aren't looking at, it is highlighted on the sidebar and in the grain list. When you look at the grain, the highlight is cleared.

screenshot from 2016-07-04 02-56-06

An app can indicate that you were explicitly mentioned/tagged/whatever. In this case, you get a bell notification. The sidebar also indicates the number of mentions outstanding for the grain. As soon as you visit the specific path in the grain where the mentions occurred (either by clicking on the notification, or otherwise), the notification is dismissed. This implies that you never receive notifications for the thing you're currently looking at.

The design is bad. Sorry, Nena is on vacation, so I did what I could. Frankly I think a complete redesign of the notification popup is in order.

screenshot from 2016-07-04 02-56-55

The following Etherpad build that uses this API. Changes to the documentation cause the basic highlight. Comments send a notification to the document owner. Replies to comments send a notification to the comment author.

/install/4fa739c2f7f6d951e6df40723ca3da6a?url=https://cac5go56oaqt71vtfj43.oasis.sandstorm.io/etherpad-notify.spk

The following are not implemented and not likely to be anytime soon, as they are big projects in themselves:

  • Notification filter settings.
  • Reviewable activity logs.

The following are not implemented but likely will come soon:

  • Desktop notification integration.
  • Email notifications / digests.

@kentonv
Copy link
Member Author

kentonv commented Jul 4, 2016

demo-server.js seems to be JSCS-broken at master.

The spot in notifications-server.js that it complains about is new in this change, but repeated invocation of jscs --fix cause it to go in a loop, never arriving at something it likes.

@kentonv
Copy link
Member Author

kentonv commented Jul 4, 2016

Note: I recommend reviewing each commit separately.

@kentonv
Copy link
Member Author

kentonv commented Jul 4, 2016

@wtogami thoughts?

# user will be prompted to share the grain with the target identity.
# - offer() the identity to the user in order to let them see the identity's profile card and
# choose to add the indentity to their contacts. You could do this e.g. when the user clicks
# on the identity's name in your app's UI. (TODO(someday): Not implemented yet.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a way to get a fresh UserInfo from an Identity? How else would an app be able to display up-to-date display names and avatars for users? (E.g. the collections app wants to display the avatar of the introducer user next to the grain.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there should, but I'll leave that for a future PR. There is a slight problem in that UserInfo contains a URL with no XSS protection -- if only it were a StaticAsset capability instead.

@zarvox
Copy link
Collaborator

zarvox commented Jul 4, 2016

Re: stylecheck failures: applying https://gist.github.com/zarvox/88accb75cd8d411b0725c4384227b6f6 appears to pass JSCS on my machine.


Template.notificationsPopup.helpers({
notifications: function () {
Meteor.call("readAllNotifications");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This call probably belongs in a Template.notificationsPopup.onCreated(...) block rather than in the middle of a template helper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not new code. I agree this code could use cleanup but I think it's out-of-scope for this PR.

This is purely a code-moving commit; there are no other changes.
This commit is purely moved code; no changes.
Using identity IDs in the notification interface was problematic because sandstorm-http-bridge truncates the identity IDs that it sends to the app, so the app actually doesn't have the full ID to send back to Sandstorm in the API.

I decided to go ahead and take the step of defining an Identity capability type to use instead, which is something that was going to happen at some point anyway. This capability is then what you put on the ActivityEvent. Over time, we should move away from identity IDs and towards capability representations, as this makes it easier for us to do things like remap identities when moving grains between servers, which may allow us to get away from the requirement that identity providers be global.

Of course, this doesn't really solve the problem for sandstorm-http-bridge users -- now they need a way to get this capability. So I extended sandstorm-http-bridge with a new feature where it can automatically save Identity capabilities when first observed, and then give the app a way to fetch those capabilities later based on (truncated) identity ID.
# - Powerbox-request an Identity to have the user choose from among their contacts. Note that the
# user will be prompted to share the grain with the target identity.
# - offer() the identity to the user in order to let them see the identity's profile card and
# choose to add the indentity to their contacts. You could do this e.g. when the user clicks
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: s/indentity/identity/

auto req = apiCap.saveRequest();
req.setCap(identity);
req.initLabel().setDefaultText("user identity");
tasks.add(req.send().then([this,textIdRef](auto result) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the -> void hint here for clang > 3.5

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.

4 participants