Skip to content

Conversation

@CA-Lee
Copy link
Contributor

@CA-Lee CA-Lee commented Oct 13, 2020

Dear Maintainers,

This PR implemented the following three APIs of webhook settings, which were also mentioned in #298 .

  • Get webhook endpoint information
  • Set webhook endpoint URL
  • Test webhook endpoint

This is my first PR on this repository, tell me if I miss something. Also, if you please, label this PR as hacktoberfest-accepted .

@louis70109
Copy link
Member

Hi @CA-Lee,
please add test cases.

Thanks for your contribution.🙂

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 14, 2020

Thanks for instantly replying, test cases were added, and CI has passed.

@louis70109
Copy link
Member

louis70109 commented Oct 18, 2020

Please fix the comments.

If you have any questions, we could discuss the comments.

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 19, 2020

Excuse me, where the comments are...? I didn't see any comment in "files changed" tab.

linebot/api.py Outdated
timeout=timeout,
)

return response.json
Copy link
Member

Choose a reason for hiding this comment

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

You need to return Webhook model, like this

linebot/api.py Outdated
timeout=timeout,
)

return response.json
Copy link
Member

Choose a reason for hiding this comment

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

You need to return Webhook model, like this

Comment on lines 49 to 50
self.assertEqual(webhook['endpoint'], 'https://example.herokuapp.com/test')
self.assertEqual(webhook['active'], 'true')
Copy link
Member

Choose a reason for hiding this comment

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

If you have Webhookmodel, the assert function would likeassertEqual(webhook.endpoint, 'http://...')`.
You can reference this

Comment on lines 52 to 56
self.assertEqual(result['success'], 'true')
self.assertEqual(result['timestamp'], '2020-09-30T05:38:20.031Z')
self.assertEqual(result['statusCode'], 200)
self.assertEqual(result['reason'], 'OK')
self.assertEqual(result['detail'], '200')
Copy link
Member

Choose a reason for hiding this comment

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

You need a TestWebhook model

@louis70109
Copy link
Member

Excuse me, where the comments are...? I didn't see any comment in "files changed" tab.

@CA-Lee sorry I forgot to push the review comment

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 19, 2020

OK, I'm working on it

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 19, 2020

All comments are fixed, CI has passed, too. I'll update the README.rst later.

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 19, 2020

README.rst was updated, the commit's timestamp is wrong due to my computer problem. Thanks for your patiently read & reply.

Copy link
Member

@louis70109 louis70109 left a comment

Choose a reason for hiding this comment

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

LGTM

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 19, 2020

Could you please to label this PR as hacktoberfest-accepted ? I'll appreciate it. :)

@louis70109
Copy link
Member

louis70109 commented Oct 20, 2020

Hi @CA-Lee ,
could you please fix the conflict information?

  • linebot/api.py
  • linebot/models/init.py
  • linebot/models/responses.py

Thanks in advance.

@CA-Lee
Copy link
Contributor Author

CA-Lee commented Oct 20, 2020

Done. :)

@louis70109 louis70109 merged commit 5ab6ba2 into line:master Oct 21, 2020
@louis70109 louis70109 mentioned this pull request Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants