-
Notifications
You must be signed in to change notification settings - Fork 3
Add routeplans endpoint #18
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
base: main
Are you sure you want to change the base?
Conversation
PR SummaryThis pull request adds support for the Route Plans API endpoints to the Ruby Onfleet SDK. The changes include:
These changes allow Ruby developers to interact with Onfleet's Route Plans API using the SDK, completing the API coverage for this resource type. |
I notice that this PR adds new CRUD operations for Route Plans in the Ruby library, but there don't appear to be any corresponding test files added or modified. Since this is adding new application logic (API endpoints), it would be beneficial to include unit tests to verify the functionality works as expected and continues to work in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 0db74c2 in 1 minute and 39 seconds. Click for details.
- Reviewed
71
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/resources/routeplans.rb:39
- Draft comment:
Inconsistent indentation: most methods indent inner lines with 2 extra spaces (total 6 relative to the class), but this method’s inner lines are indented further. Please adjust to match the rest of the file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment points out a real formatting inconsistency. However, this is a style/formatting issue that would typically be caught and fixed automatically by code formatters or linters. The rules state we should not make purely informative comments or comments about obvious issues. The indentation inconsistency is real and could make the code less readable. Maybe this codebase doesn't have automated formatting tools in place? Even if there are no automated tools, indentation issues are obvious visual problems that don't require a comment to point out. The PR author can see this directly. This comment should be deleted as it points out an obvious formatting issue that doesn't require explanation.
2. lib/resources/routeplans.rb:40
- Draft comment:
The path string here includes a leading slash, unlike other endpoints. For consistency, remove the leading slash from the route path. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_72Oj3oQ5ac9bmJMt
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 6a18af4 in 1 minute and 20 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/onfleet.rb:17
- Draft comment:
This change appears to be a no-op (the require for './resources/workers' is unchanged). Please remove any redundant whitespace or non-functional modifications. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is technically correct - there is trailing whitespace added that serves no purpose. However, this is an extremely minor formatting issue that would likely be caught by linters or formatters. The comment doesn't point out any real code issues or suggest meaningful improvements. The comment is factually accurate. Perhaps keeping it could help maintain consistent code formatting. While accurate, comments about minor whitespace issues are too trivial and would create noise in the review. This should be handled by automated tools rather than manual comments. Delete this comment as it points out an extremely minor formatting issue that doesn't impact functionality and would be better handled by automated tools.
2. lib/onfleet.rb:17
- Draft comment:
There's an extra trailing space at the end of the line (require_relative './resources/workers'
). Consider removing the trailing whitespace. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While trailing whitespace is generally undesirable, this is an extremely minor stylistic issue that would typically be handled by editor settings or linting rules. It doesn't affect functionality and isn't worth a PR comment. Most teams have automated tools to handle whitespace issues. Perhaps trailing whitespace could cause issues in some edge cases or with certain tools? Maybe the team has specific formatting standards they want to enforce manually? Even if there are edge cases, this is still too minor for a manual review comment. Such formatting issues should be caught by automated tools, not human reviewers. This comment should be removed as it addresses an extremely minor stylistic issue that would be better handled by automated tooling.
Workflow ID: wflow_fhhdnfbEQnD3OzFn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed cc11878 in 38 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/resources/routeplans.rb:40
- Draft comment:
The path now omits the leading slash to be consistent with other endpoint definitions. Please ensure that Onfleet.request expects a relative path. This change aligns with the module's overall URL pattern. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking the PR author to ensure that Onfleet.request expects a relative path, which violates the rule against asking the author to confirm or ensure behavior. However, it also provides a suggestion about aligning with the module's URL pattern, which is useful. The comment is partially useful but mostly violates the rules.
Workflow ID: wflow_wInsXT29ISQZe4tB
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed efd9942 in 2 minutes and 12 seconds. Click for details.
- Reviewed
212
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
8
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/onfleet.rb:17
- Draft comment:
Removed trailing whitespace from the require statement for './resources/workers'. Nice cleanup. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. lib/resources/destinations.rb:17
- Draft comment:
Removed the redundant 'ACTION: still needs to be tested' comment from the match_metadata method. Ensure corresponding tests are in place. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. lib/resources/organizations.rb:17
- Draft comment:
Removed redundant testing comment from the insert_task method. Ensure that tests are implemented for this endpoint. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. lib/resources/recipients.rb:39
- Draft comment:
Removed redundant testing comment from the match_metadata method. Make sure tests cover this functionality. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. lib/resources/routeplans.rb:38
- Draft comment:
Removed extraneous whitespace in the add_tasks_to_routeplan method signature. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. lib/resources/tasks.rb:15
- Draft comment:
Removed multiple redundant 'ACTION: still needs to be tested' comments from Tasks methods. Confirm that proper tests exist for batch creation, async creation, complete, auto_assign, and match_metadata. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. lib/resources/webhooks.rb:10
- Draft comment:
Removed redundant testing comments from the Webhooks methods. Ensure appropriate tests are in place to verify webhook functionality. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. lib/resources/workers.rb:51
- Draft comment:
Updated action comments to indicate that get_by_location, insert_task, and match_metadata have been tested. This clarifies their implementation status. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_PWVC5nhABuWGhAjy
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
if query_parameters_hash | ||
query_parameters = URI.encode_www_form(query_parameters_hash) | ||
end | ||
path = "teams/#{id}/tasks?#{query_parameters}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In get_unassigned_tasks, if no query parameters are provided, the URL will end with '?nil'. Consider conditionally appending the query string only when query parameters exist. For example:
path = "teams/#{id}/tasks"
path += "?#{URI.encode_www_form(query_parameters_hash)}" if query_parameters_hash
path = "teams/#{id}/tasks?#{query_parameters}" | |
path = "teams/#{id}/tasks" + (query_parameters ? "?#{query_parameters}" : "") |
This comment was generated because it violated a code review rule: mrule_MmFUQCGeCc2tGBoo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 45409ed in 47 seconds. Click for details.
- Reviewed
28
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/resources/workers.rb:52
- Draft comment:
Removed an outdated comment about testing. Ensure that proper tests exist instead of relying on inline TODOs. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. lib/resources/workers.rb:90
- Draft comment:
Removed an outdated testing reminder comment before insert_task. Verify that tests cover this endpoint. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. lib/resources/workers.rb:97
- Draft comment:
Removed an outdated testing reminder comment before match_metadata. Consider adding verification tests if missing. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_Z1xKaeYJnjD9Ijkl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Adds route plans endpoint with CRUD operations and removes outdated comments across multiple files.
routeplans.rb
with CRUD operations:create
,get
,get_by_id
,update
,add_tasks_to_routeplan
,delete_one
.routeplans.rb
intoonfleet.rb
.destinations.rb
,organizations.rb
,recipients.rb
,tasks.rb
,teams.rb
,webhooks.rb
,workers.rb
.get_unassigned_tasks
inteams.rb
to include optional query parameters.This description was created by
for 45409ed. You can customize this summary. It will automatically update as commits are pushed.