Fix RPC's success code in case of UNSUPPORTED_RESOURCE by AKalinich-Luxoft · Pull Request #1575 · smartdevicelink/sdl_core

AKalinich-Luxoft · 2017-05-29T13:19:35Z

Fixed Alert, AlertManeuver, PerformAudioPassThru success code in case of UNSUPPORTED_RESOURCE. The root cause of problem here is that SDL does not check specific case for this result code for RPC with two or more interfaces and just return wrong success result code.

In this pull request:

Added overriden functions PrepareResultForMobileResponse/PrepareResultCodeForResponse
to cover specific cases for listed RPCs
Updated unit test expectations for PerformAudioPassThruRequestTest

Note
Some changes are from PR #1567 and will disappear after rebase.

AKalinich-Luxoft · 2017-05-29T13:21:47Z

@AByzhynar, @LitvinenkoIra, @LuxoftAKutsan, @dev-gh, @anosach-luxoft please review

AByzhynar · 2017-05-30T09:35:08Z

src/components/application_manager/include/application_manager/commands/command_request_impl.h

   */
-  bool PrepareResultForMobileResponse(ResponseInfo& out_first,
-                                      ResponseInfo& out_second) const;
+  virtual bool PrepareResultForMobileResponse(ResponseInfo& out_first,


@AKalinich-Luxoft Please rename parameters like you did in IsResultCodeUnsupported(const ResponseInfo& first_iface_response...

@AByzhynar done in 596d8b9

@AKalinich-Luxoft No requested changes provided

@AByzhynar sorry, wrong commit number. Please see 9e2ce85

AByzhynar · 2017-05-30T09:35:17Z

src/components/application_manager/include/application_manager/commands/command_request_impl.h

   * @return resulting code for sending to mobile application.
   */
-  mobile_apis::Result::eType PrepareResultCodeForResponse(
+  virtual mobile_apis::Result::eType PrepareResultCodeForResponse(


@AKalinich-Luxoft Please rename parameters like you did in IsResultCodeUnsupported(const ResponseInfo& first_iface_response...

@AByzhynar done in 596d8b9

@AKalinich-Luxoft No requested changes provided

@AByzhynar sorry, wrong commit number. Please see 9e2ce85

JenkinsSDLOnCloud · 2017-05-30T11:18:36Z

ATF failed http://opensdl-jenkins.luxoft.com:8080/job/OpenSDL_Develop_PR/job/OpenSDL_develop_ATF/325/

LuxoftAKutsan · 2017-05-31T09:02:32Z

src/components/application_manager/include/application_manager/commands/command_request_impl.h

-  bool PrepareResultForMobileResponse(ResponseInfo& out_first,
-                                      ResponseInfo& out_second) const;
+  virtual bool PrepareResultForMobileResponse(
+      ResponseInfo& first_iface_response,


@AKalinich-Luxoft it is it out parameters please mark them as out parameters

@LuxoftAKutsan done in 28af6b8

LuxoftAKutsan · 2017-05-31T09:05:55Z

src/components/application_manager/src/commands/command_request_impl.cc

-         (first.is_unsupported_resource && second.is_unsupported_resource);
+bool IsResultCodeUnsupported(const ResponseInfo& first_iface_response,
+                             const ResponseInfo& second_iface_response) {
+  return ((first_iface_response.is_ok ||


@AKalinich-Luxoft please split this condition to bool const variables for understanding.

@LuxoftAKutsan done in 28af6b8

LuxoftAKutsan · 2017-05-31T09:06:15Z

src/components/application_manager/src/commands/command_request_impl.cc

-  out_second.is_invalid_enum =
-      hmi_apis::Common_Result::INVALID_ENUM == out_second.result_code;
+  second_iface_response.is_invalid_enum =
+      hmi_apis::Common_Result::INVALID_ENUM ==


@AKalinich-Luxoft auto style fix?

@LuxoftAKutsan yes, this code was formatted with clang-format-3.6

Fixed AlertManeuver success result in case of UNSUPPORTED_RESOURCE. The problem was that SDL does not check success result in this specific case. In this commit: - Added check for UNSUPPORTED_RESOURCE case - All specific checks were moved to PrepareResultForMobileResponse/PrepareResultCodeForResponse function - PrepareResultForMobileResponse is currently virtual

Fixed Alert, AlertManeuver, PerformAudioPassThru success code in case of UNSUPPORTED_RESOURCE. The root cause of problem here is that SDL does not check specific case for this result code for RPC with two or more interfaces and just return wrong success result code. In this commit: - Added overriden functions PrepareResultForMobileResponse/PrepareResultCodeForResponse to cover specific cases for listed RPCs

Updated unit test expectations for PerformAudioPassThruRequestTest

AKalinich-Luxoft · 2017-06-02T05:56:13Z

@LuxoftAKutsan please finish you review

JenkinsSDLOnCloud · 2017-10-07T08:36:08Z

Can one of the admins verify this patch?

JenkinsSDLOnCloud · 2017-10-10T16:09:06Z

Can one of the admins verify this patch?

JenkinsSDLOnCloud · 2017-10-13T10:16:11Z

Can one of the admins verify this patch?

dev-gh approved these changes May 29, 2017

View reviewed changes

AKalinich-Luxoft mentioned this pull request May 30, 2017

Fix response to SetGlobalProperties in case of UNSUPPORTED TTS #1574

Open

AByzhynar suggested changes May 30, 2017

View reviewed changes

AKalinich-Luxoft force-pushed the fix/fix_alert_performaudiopassthru_success_result branch from 7930ff8 to 596d8b9 Compare May 30, 2017 10:19

LitvinenkoIra approved these changes May 30, 2017

View reviewed changes

AByzhynar approved these changes May 30, 2017

View reviewed changes

LuxoftAKutsan suggested changes May 31, 2017

View reviewed changes

AKalinich-Luxoft added 3 commits May 31, 2017 18:34

Update unit test expectations

abdf536

Updated unit test expectations for PerformAudioPassThruRequestTest

AKalinich-Luxoft force-pushed the fix/fix_alert_performaudiopassthru_success_result branch from 596d8b9 to abdf536 Compare May 31, 2017 15:35

LuxoftAKutsan approved these changes Jun 6, 2017

View reviewed changes

jacobkeeler closed this Sep 28, 2022

Conversation

AKalinich-Luxoft commented May 29, 2017

Uh oh!

AKalinich-Luxoft commented May 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JenkinsSDLOnCloud commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AKalinich-Luxoft commented Jun 2, 2017

Uh oh!

JenkinsSDLOnCloud commented Oct 7, 2017

Uh oh!

JenkinsSDLOnCloud commented Oct 10, 2017

Uh oh!

JenkinsSDLOnCloud commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants