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 notification related endpoints and editorconfig #12

Merged
merged 8 commits into from
Jul 6, 2018

Conversation

singhpratyush
Copy link
Contributor

@singhpratyush singhpratyush commented Jun 24, 2018

  • Use Authorization header for taking steemconnect access token
  • Use expo sdk to validate tokens
  • Return notifications at /notifications
  • Send notifications to users using expo SDK
  • Add editorconfig

Related: busyorg/busy#1872.


const router = express.Router();

router.get('/', async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think receiving notifications with http transport is not used anymore, so we may not need this route. Can you confirm @jm90m ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are removing this, the validTokenMiddleware can be applied to the whole /notifications route.

@bonustrack
Copy link
Contributor

I think what we miss is a way to send push notification to bSteem users when we stream them here: https://github.com/singhpratyush/busy-api/blob/3334b319b1e84fa190ab6e3ea8d099667852601c/server.js#L273

@singhpratyush
Copy link
Contributor Author

singhpratyush commented Jun 25, 2018

I think what we miss is a way to send push notification to bSteem users

@bonustrack: I was planning to add it in further patches. But I can add it here if we want.

@singhpratyush singhpratyush force-pushed the expo-registration branch 3 times, most recently from 17ab982 to 7226c93 Compare June 25, 2018 06:19
@Sekhmet
Copy link
Contributor

Sekhmet commented Jun 25, 2018

I feel like if we want to merge it to master we should make it all in one PR, or we can create new branch and apply multiple PRs there.

@singhpratyush
Copy link
Contributor Author

I'll add send notification hooks in this patch itself.

- Use Authorization header for taking steemconnect access token
- Use expo sdk to validate tokens
- Return notifications at /notifications
@singhpratyush singhpratyush force-pushed the expo-registration branch 2 times, most recently from 34d11ac to a5a7761 Compare July 4, 2018 06:48
@singhpratyush
Copy link
Contributor Author

@bonustrack @Sekhmet: I've added the code to send push notifications. Sorry for the delay.

@@ -0,0 +1,87 @@
const Expo = require('expo-server-sdk');
Copy link
Contributor

Choose a reason for hiding this comment

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

We use 2 spaces as the indentation. This file uses tabs. Can you update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll also add an editorconfig.

message = {
body: `${data.author} mentioned you in a ` + (data.is_root_post ? 'post.' : 'comment.'),
};
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon there.

@singhpratyush singhpratyush changed the title Add notification related endpoints Add notification related endpoints and editorconfig Jul 4, 2018
Copy link
Contributor

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

Awesome job.

@Sekhmet Sekhmet merged commit 346b6ee into busyorg:master Jul 6, 2018
@singhpratyush
Copy link
Contributor Author

Yayy 🎉

@singhpratyush singhpratyush deleted the expo-registration branch July 9, 2018 10:46
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.

3 participants