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

Feat/sync issues status with GitHub webhooks #501

Conversation

SavvyShah
Copy link
Contributor

Description

This allows to sync the issue status on Github with our website.

Changes

  • /modules/app/controllers/webhooks.js
  • /test/webhook.test.js

Issue

#484

Impacted Area

I've added update query so database gets updated.
All the pages where issue status is displayed

Steps to test

Steps needed to reproduce the scenario from this change to validate if the pull request solve this issue

  • I have added the test in /test/webhook.test.js
  • Similar to the test you can create your own and test if the thing works.

Some other steps you can follow(requires postman)

  • Start the backend server.
  • (Optional)Start pgAdmin to see change in database.
  • Send post requests through postman with the data as json.

Before

Screenshot from the state before

After

Screenshot from the state after your Pull Request

@alexanmtz
Copy link
Member

@ShubhamCanMakeCommit please check the build failing:
https://circleci.com/gh/worknenjoy/gitpay/1143

@SavvyShah
Copy link
Contributor Author

It is not showing when I run the tests. I will try to solve. Maybe you can tell what might be going wrong.

@SavvyShah
Copy link
Contributor Author

Okay, there is a problem in my test. I will solve that.

@alexanmtz
Copy link
Member

You can see that maybe is missing the authorization when calling the webhook from the tests as the following tests:
https://github.com/ShubhamCanMakeCommit/gitpay/blob/feat/sync-issues-status-with-github-webhooks/test/webhook.test.js#L407

Copy link
Member

@alexanmtz alexanmtz left a comment

Choose a reason for hiding this comment

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

The solution is good! Some improvements to do the update and return the updated model.

modules/app/controllers/webhook.js Outdated Show resolved Hide resolved
Copy link
Member

@alexanmtz alexanmtz left a comment

Choose a reason for hiding this comment

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

@ShubhamCanMakeCommit Once you returned the update method with the returning parameter, you will no longer need to call a find and you need to change the checks accordingly. Please make the changes and test again, we are almost there ✋

@alexanmtz
Copy link
Member

Thanks @ShubhamCanMakeCommit , now we need to notify our users as described on the issue. When update, we should call a new mail function with the following template:

Hello {{name}},

The issue {{title}} - {{url}} was closed on Github, so we closed on Gitpay. 

You can change the issue status if you want.
 
Thanks,
Gitpay Team

----------------------
https://gitpay.me (Web)
tarefas@gitpay.me (Email)
Worknenjoy Inc., 2035 Sunset Lake Road #Suite B-2, Newark, DE 19702 (Mail)

So check the existent mail functions to create new notification templates.

You think is possible to do that?

@SavvyShah
Copy link
Contributor Author

SavvyShah commented Mar 18, 2020 via email

@alexanmtz
Copy link
Member

Great @ShubhamCanMakeCommit , now we can proceed and merge 🚀

@alexanmtz alexanmtz merged commit 0939b4e into worknenjoy:master Mar 19, 2020
@alexanmtz
Copy link
Member

@ShubhamCanMakeCommit , I will test creating an issue and closing to see it in action 👍

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.

None yet

2 participants