Fix AlertManeuver success result in case of UNSUPPORTED_RESOURCE by AKalinich-Luxoft · Pull Request #1567 · smartdevicelink/sdl_core

AKalinich-Luxoft · 2017-05-26T09:43:45Z

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 pull request:

Added check for UNSUPPORTED_RESOURCE case
All specific checks were moved to PrepareResultForMobileResponse function
PrepareResultForMobileResponse is currently virtual

AKalinich-Luxoft · 2017-05-26T09:44:26Z

@anosach-luxoft, @AByzhynar, @dev-gh, @VProdanov please review

dev-gh · 2017-05-26T10:06:38Z

src/components/application_manager/src/commands/mobile/alert_maneuver_request.cc

+  if (mobile_apis::Result::UNSUPPORTED_RESOURCE == result_code) {
+    // According to APPLINK-19582 should be success:true in this case
+    return true;
+  }


@AKalinich-Luxoft
I think that is not a good approach to add more if() as this spreads special cases across different places. I propose to do changes similar to #1562 so then logic may be extended to cover specific cases for this particular command.
Also I suggest to move within these overridden method logic from lines 201-209

@dev-gh done as in your pull request. All specific case checks were moved from current function to virtual one. Please see 75e8ed3

AByzhynar · 2017-05-26T13:48:20Z

src/components/application_manager/src/commands/mobile/alert_maneuver_request.cc

      MergeInfos(navigation_alert_info, info_navi_, tts_alert_info, info_tts_);
+
+  if (mobile_apis::Result::UNSUPPORTED_RESOURCE == result_code) {
+    // According to APPLINK-19582 should be success:true in this case


@AKalinich-Luxoft Remove JIRA task from comment. It is prohibited to specify such things in OpenSDL.

@AByzhynar done in 75e8ed3

ghost · 2017-05-29T07:18:36Z

src/components/application_manager/src/commands/mobile/alert_maneuver_request.cc

-    return result;
+    tts_alert_info.result_code = hmi_apis::Common_Result::WARNINGS;
+  } else if (IsResultCodeUnsupported(navigation_alert_info, tts_alert_info)) {
+    return true;


@AKalinich-Luxoft maybe result = true..

@VProdanov I think here is no difference because result will be returned as true on the next line in this case

@AKalinich-Luxoft yes, its for consistency..

@VProdanov fixed in 75e8ed3

ghost · 2017-05-29T07:22:24Z

...nts/application_manager/include/application_manager/commands/mobile/alert_maneuver_request.h

+   */
+  bool PrepareResultForMobileResponse(
+      ResponseInfo& navigation_alert_info,
+      ResponseInfo& tts_alert_info) const OVERRIDE;


@AKalinich-Luxoft FINAL?

@VProdanov fixed in 75e8ed3

AByzhynar · 2017-05-29T10:32:30Z

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

+ * interface that returns third response
+ * @return true if overall result code is UNSUPPORTED_RESOURCE otherwise false
+ */
+bool IsResultCodeUnsupported(const ResponseInfo& first,


@AKalinich-Luxoft I would rename first -> first_iface_response and so on...

@AByzhynar done in 9b902f4

AByzhynar · 2017-05-30T09:37:09Z

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 9e2ce85

AByzhynar · 2017-05-30T09:37: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 9e2ce85

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

JenkinsSDLOnCloud · 2017-05-31T16:24:41Z

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

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

Can one of the admins verify this patch?

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

Can one of the admins verify this patch?

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

Can one of the admins verify this patch?

dev-gh reviewed May 26, 2017

View reviewed changes

AByzhynar suggested changes May 26, 2017

View reviewed changes

AKalinich-Luxoft force-pushed the fix/fix_alert_maneuer_result_code branch from efb2f68 to 553abdd Compare May 29, 2017 07:04

ghost reviewed May 29, 2017

View reviewed changes

ghost approved these changes May 29, 2017

View reviewed changes

AKalinich-Luxoft force-pushed the fix/fix_alert_maneuer_result_code branch from 20a8cd4 to 75e8ed3 Compare May 29, 2017 09:22

AByzhynar approved these changes May 29, 2017

View reviewed changes

AByzhynar suggested changes May 29, 2017

View reviewed changes

dev-gh approved these changes May 29, 2017

View reviewed changes

AKalinich-Luxoft force-pushed the fix/fix_alert_maneuer_result_code branch from 75e8ed3 to 9b902f4 Compare May 29, 2017 12:55

This was referenced May 29, 2017

Fix RPC's success code in case of UNSUPPORTED_RESOURCE #1575

Closed

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_maneuer_result_code branch from 9b902f4 to 9e2ce85 Compare May 30, 2017 10:06

AKalinich-Luxoft force-pushed the fix/fix_alert_maneuer_result_code branch from 9e2ce85 to d92f767 Compare May 31, 2017 15:27

AByzhynar approved these changes Jun 6, 2017

View reviewed changes

jacobkeeler closed this Sep 28, 2022

Conversation

AKalinich-Luxoft commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AKalinich-Luxoft commented May 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AKalinich-Luxoft May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AKalinich-Luxoft May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ghost May 29, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AKalinich-Luxoft May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AKalinich-Luxoft May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 31, 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!

5 participants

AKalinich-Luxoft commented May 26, 2017 •

edited

Loading

AKalinich-Luxoft May 29, 2017 •

edited

Loading

AKalinich-Luxoft May 29, 2017 •

edited

Loading

ghost May 29, 2017 •

edited by ghost

Loading

AKalinich-Luxoft May 29, 2017 •

edited

Loading

AKalinich-Luxoft May 29, 2017 •

edited

Loading