Skip to content

GCW-3407 Gogox Gem Integration and Transport APIs - #1195

Open
swatijadhav wants to merge 20 commits into
masterfrom
GCW-3407
Open

GCW-3407 Gogox Gem Integration and Transport APIs#1195
swatijadhav wants to merge 20 commits into
masterfrom
GCW-3407

Conversation

@swatijadhav

@swatijadhav swatijadhav commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

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

@steveyken steveyken left a comment

Copy link
Copy Markdown
Member

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

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

class ValueError < StandardError; end

Copy link
Copy Markdown
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

Comment thread app/services/transport_service.rb Outdated
Comment thread app/services/transport_service.rb Outdated
Co-authored-by: bharat619 <37997792+bharat619@users.noreply.github.com>

@steveyken steveyken left a comment

Copy link
Copy Markdown
Member

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

Comment thread app/controllers/api/v1/transports_controller.rb
Comment thread app/controllers/api/v1/transports_controller.rb
Comment thread app/controllers/api/v1/transports_controller.rb Outdated
end

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

Copy link
Copy Markdown
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.

Comment thread db/migrate/20210111115250_create_transport_orders.rb Outdated
@swatijadhav
swatijadhav requested a review from steveyken February 9, 2021 16:24

@steveyken steveyken left a comment

Copy link
Copy Markdown
Member

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.

Comment thread app/services/transport_service.rb
Comment thread app/controllers/api/v1/transports_controller.rb
status: response["status"],
scheduled_at: Time.at(response["pickup"]["schedule_at"]).in_time_zone,
metadata: response,
source_id: @params[:source_id],

Copy link
Copy Markdown
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

Comment thread app/services/transport_service.rb Outdated
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