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

GCW-3407 Gogox Gem Integration and Transport APIs #1195

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

swatijadhav
Copy link
Contributor

@swatijadhav swatijadhav commented Jan 12, 2021

Ticket Link:

https://jira.crossroads.org.hk/browse/GCW-3407

What does this PR do?

Gogox Gem Integration and Transport APIs

NOTE: webhook implementation is still WIP

Copy link
Member

@steveyken steveyken left a comment

Choose a reason for hiding this comment

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

Great work. Good to see this taking shape and getting ready to integrate the new GGX booking

app/controllers/api/v1/transports_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v1/transports_controller.rb Outdated Show resolved Hide resolved
app/services/gogox.rb Outdated Show resolved Hide resolved
app/services/transport_service.rb Outdated Show resolved Hide resolved
spec/services/gogox_spec.rb Outdated Show resolved Hide resolved
config/routes.rb Show resolved Hide resolved
app/services/transport_service.rb Outdated Show resolved Hide resolved
app/models/transport_provider_order.rb Outdated Show resolved Hide resolved
@swatijadhav swatijadhav marked this pull request as ready for review January 18, 2021 14:06
app/services/gogox.rb Outdated Show resolved Hide resolved
@time.to_i
end

class ValueError < StandardError; end
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of just value error defining here, create a error in errors.rb
something like TransportationServiceError

Copy link
Member

@steveyken steveyken left a comment

Choose a reason for hiding this comment

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

Can you remove ngrok binary?

Great work, almost complete

app/controllers/api/v1/transports_controller.rb Outdated Show resolved Hide resolved
end

api :POST, '/v1/transports/update_hook', "Webhook to update transport status"
def update_hook
Copy link
Member

Choose a reason for hiding this comment

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

When implemented, we should use our own authorization check here. E.g. webhook can be /api/v1/transports/update_hook?shared_key=ds23f934dfs&param1=df&param2=23

Then check shared_key is as expected.

db/migrate/20210111115250_create_transport_orders.rb Outdated Show resolved Hide resolved
Copy link
Member

@steveyken steveyken left a comment

Choose a reason for hiding this comment

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

There's a few issues with un-sanitized user input which need to be addressed. Otherwise, looking good.

app/services/transport_service.rb Show resolved Hide resolved
status: response["status"],
scheduled_at: Time.at(response["pickup"]["schedule_at"]).in_time_zone,
metadata: response,
source_id: @params[:source_id],
Copy link
Member

Choose a reason for hiding this comment

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

As noted in the controller, this is where source_id and source_type is being overridden by user params rather than the sanitized CanCanCan object. Use @transports instead

app/services/transport_service.rb Outdated Show resolved Hide resolved
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