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

create validate_rich_menu method #259

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

kenta-takeuchi
Copy link
Contributor

i'd like to write a new validate rich menu api.

Copy link
Member

@zenizh zenizh left a comment

Choose a reason for hiding this comment

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

Thank you for your sending this pull request! I made a few comments, so please check them in your spare time.

@@ -547,6 +547,18 @@ def create_rich_menu(rich_menu)
post(endpoint, endpoint_path, rich_menu.to_json, credentials)
end

# Validate a rich menu
Copy link
Member

Choose a reason for hiding this comment

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

[nice to have]

This endpoint validates an object of the rich menu.

Suggested change
# Validate a rich menu
# Validate a rich menu object

# @param rich_menu [Hash] The rich menu represented as a rich menu object
#
# @return [Net::HTTPResponse]
def validate_rich_menu(rich_menu)
Copy link
Member

Choose a reason for hiding this comment

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

[must fix]

Unlike other endpoints, this endpoint validates an object, not a rich menu. So the name should be represented more accurately.

Suggested change
def validate_rich_menu(rich_menu)
def validate_rich_menu_object(rich_menu)

Comment on lines 98 to 104
it 'validates a rich menu' do
uri_template = Addressable::Template.new Line::Bot::API::DEFAULT_ENDPOINT + '/bot/richmenu/validate'
stub_request(:post, uri_template).to_return(body: '{}', status: 200)

client.validate_rich_menu(JSON.parse(RICH_MENU_CONTENT))
expect(WebMock).to have_requested(:post, Line::Bot::API::DEFAULT_ENDPOINT + '/bot/richmenu/validate')
end
Copy link
Member

Choose a reason for hiding this comment

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

[must fix]

This should be changed according to the change of the method name.

Suggested change
it 'validates a rich menu' do
uri_template = Addressable::Template.new Line::Bot::API::DEFAULT_ENDPOINT + '/bot/richmenu/validate'
stub_request(:post, uri_template).to_return(body: '{}', status: 200)
client.validate_rich_menu(JSON.parse(RICH_MENU_CONTENT))
expect(WebMock).to have_requested(:post, Line::Bot::API::DEFAULT_ENDPOINT + '/bot/richmenu/validate')
end
it 'validates a rich menu object' do
uri_template = Addressable::Template.new Line::Bot::API::DEFAULT_ENDPOINT + '/bot/richmenu/validate'
stub_request(:post, uri_template).to_return(body: '{}', status: 200)
client.validate_rich_menu_object(JSON.parse(RICH_MENU_CONTENT))
expect(WebMock).to have_requested(:post, Line::Bot::API::DEFAULT_ENDPOINT + '/bot/richmenu/validate')
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review.
I understand your comments and I will fix it later.

@kenta-takeuchi
Copy link
Contributor Author

All comments have been addressed.
Please review them again when you have time.

Copy link
Member

@zenizh zenizh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zenizh zenizh merged commit a731a26 into line:master Jul 13, 2022
@kenta-takeuchi kenta-takeuchi deleted the validate_rich_menu branch July 13, 2022 02:10
@zenizh zenizh mentioned this pull request Jul 13, 2022
@zenizh
Copy link
Member

zenizh commented Jul 13, 2022

@kenta-takeuchi san
FYI: If you write "Resolves #256" instead of "Solves #256" in the description, the issue is automatically closed when the pull request is merged 👍

@kenta-takeuchi
Copy link
Contributor Author

@zenizh
Thanks for your information.
I'll try to link issues with correct description.

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

3 participants