Fix sub menues deletion wrong order #3748

AKalinich-Luxoft · 2021-08-02T17:01:51Z

Fixes #3747

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Can be reproduced manually:

Reproduction Steps

SDL and HMI are started
Mobile device connected
Mobile app is registered and activated
SubMenu item is added
AddSubMenu("menuID":1, "menuName":"Submenu Name 1")`
Nested submenu items are added (each submenu is nested in the previous submenu)
Send AddSubMenu(parentID=1, "menuID":2, "menuName":"Submenu Name 2")
Send AddSubMenu(parentID=2, "menuID":3, "menuName":"Submenu Name 3")
Send AddSubMenu(parentID=3, "menuID":4, "menuName":"Submenu Name 4")
AddCommand item is added to nested SubMenu
Send AddCommand("menuParams: { "parentID:4, "menuName":"Item to Add 1", "position":0)
Delete submenu item (item is menu-tree branch)
Send DeleteSubMenu("menuID":1)

Summary

The issue which was observed is that SDL sends DeleteSubMenu request to HMI for a "parent node" submenu and then sends
subsequent DeleteSubMenu requests recursively for all child nodes starting from the bottom one. Such an order confuses HMI because deletion of parent node may automatically trigger deletion of all its child nodes on the HMI side so subsequent requests from SDL for a child node might be rejected.
To make sure that HMI removes all the hierarchy properly, SDL should start deletion sequence from the bottom nodes and delete the parent node in the end. To resolve that issue, DeleteSubMenu request logic was updated. Currently, SDL recursively iterates through the submenu and command nodes and collects them in a queue.
Once the queue is ready, SDL sends requests one by one, waiting for an HMI response for each sent request before sending the
subsequent. Once the responses to all requests are received, SDL sends the final response to the mobile app and finalizes the
command.
Also updated DeleteSubMenu unit tests to cover new logic.

CLA

I have signed the CLA

The issue which was observed is that SDL sends `DeleteSubMenu` request to HMI for a "parent node" submenu and then sends subsequent `DeleteSubMenu` requests recursively for all child nodes starting from the bottom one. Such order confuses HMI because deletion of parent node may automatically trigger deletion of all its child nodes on HMI side so subsequent requests from SDL for a child nodes might be rejected. To make sure that HMI removes all the hierarchy properly, SDL should start deletion sequence from the bottom nodes and delete the parent node in the end. To resolve that issue, DeleteSubMenu request logic was updated. Currently, SDL recursively iterates through the submenu and command nodes and collects them in a queue. Once queue is ready, SDL sends requests one by one, waiting for a HMI response for each sent request before sending the subsequent. Once the responses to all requests are received, SDL sends the final response to mobile app and finalizes the command.

AKalinich-Luxoft · 2021-08-02T17:02:28Z

@theresalech this PR is ready for Livio review

.../rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/delete_sub_menu_request.h

...pplication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/delete_sub_menu_request.cc

.../application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/delete_sub_menu_test.cc

jacobkeeler · 2021-08-04T21:43:15Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/delete_sub_menu_test.cc

+    return smartObjectPtr;
+  }
+
+  void RunDeleteSubMenuHappyPath(


I think these functions could definitely use descriptions, what exactly they are doing isn't entirely clear.

@jacobkeeler so I made some refactoring and split these functions on more atomic with more obvious names:

SetUp* functions for setting up the expectations for a corresponding test case

Prepare* functions for preparing event object

Generate* functions for generating corresponding smart objects

See 0b07d16

jacobkeeler

Also looks like the following ATF tests are failing because of the change in message order:

FAILED test_scripts/API/Additional_Submenus/003_delete_nested_submenu_with_nested_submenu_and_commands.lua
FAILED test_scripts/API/Additional_Submenus/004_delete_top_level_submenu_with_nested_submenus_and_commands.lua

.../application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/delete_sub_menu_test.cc

.../rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/delete_sub_menu_request.h

jacobkeeler · 2021-08-09T18:53:37Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/delete_sub_menu_test.cc

+  EXPECT_CALL(
+      mock_rpc_service_,
+      ManageMobileCommand(MobileResultCodeIs(mobile_apis::Result::SUCCESS),
+                          am::commands::Command::SOURCE_SDL));


The name of this test specifically says UNSUCCESS, why would we see a SUCCESS response?

@jacobkeeler looks like fix changed the behavior and now SDL sends the response even if the app is not registered. I updated the fix to keep the behavior that was before. This unit test has been updated accordingly. See 0aec1a7

jacobkeeler · 2021-08-09T19:16:53Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/delete_sub_menu_test.cc

@@ -216,174 +432,219 @@ TEST_F(DeleteSubMenuRequestTest, Run_FindSubMenuFalse_UNSUCCESS) {
 }

 TEST_F(DeleteSubMenuRequestTest, Run_SendHMIRequest_SUCCESS) {


These two Run_* tests seems to just be subsets of later OnEvent_* tests

@jacobkeeler these two tests were removed. See 0aec1a7

AKalinich-Luxoft · 2021-08-10T13:11:23Z

Also looks like the following ATF tests are failing because of the change in message order:

FAILED test_scripts/API/Additional_Submenus/003_delete_nested_submenu_with_nested_submenu_and_commands.lua
FAILED test_scripts/API/Additional_Submenus/004_delete_top_level_submenu_with_nested_submenus_and_commands.lua

@jacobkeeler right, these two scripts should be updated to work with the new order of the commands.
Appropriate changes were done in smartdevicelink/sdl_atf_test_scripts#2569
This PR has been linked to GitHub issue as well and should be merged together with that PR.

jacobkeeler · 2021-08-10T13:51:20Z

...pplication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/delete_sub_menu_request.cc

+        auto app = application_manager_.application(
+            msg_params[strings::app_id].asUInt());


Suggested change

auto app = application_manager_.application(

msg_params[strings::app_id].asUInt());

auto app = application_manager_.application(connection_key());

While they should act the same, I think it's safer and easier to use the original connection key here

@jacobkeeler good point. Fixed in b4ba264

jacobkeeler · 2021-08-10T13:51:52Z

...pplication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/delete_sub_menu_request.cc

+        auto app = application_manager_.application(
+            msg_params[strings::app_id].asUInt());


Suggested change

auto app = application_manager_.application(

msg_params[strings::app_id].asUInt());

auto app = application_manager_.application(connection_key());

While they should act the same, I think it's safer to use the original connection key here

@jacobkeeler good point. Fixed in b4ba264

jacobkeeler · 2021-08-10T13:52:08Z

...pplication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/delete_sub_menu_request.cc

+        auto app = application_manager_.application(
+            msg_params[strings::app_id].asUInt());


Suggested change

auto app = application_manager_.application(

msg_params[strings::app_id].asUInt());

auto app = application_manager_.application(connection_key());

While they should act the same, I think it's safer to use the original connection key here

@jacobkeeler good point. Fixed in b4ba264

AKalinich-Luxoft added 3 commits July 27, 2021 19:45

Updated DeleteSubMenu unit tests to cover new logic

24d4b36

fixup! Updated DeleteSubMenu unit tests to cover new logic

9a2ff01

AKalinich-Luxoft mentioned this pull request Aug 2, 2021

Fixed response for DeleteCommand request smartdevicelink/sdl_hmi#579

Closed

1 task

jacobkeeler requested changes Aug 4, 2021

View reviewed changes

Address Livio comments

0b07d16

jacobkeeler requested changes Aug 9, 2021

View reviewed changes

Address Livio comments

0aec1a7

jacobkeeler requested changes Aug 10, 2021

View reviewed changes

Address Livio comments

b4ba264

jacobkeeler approved these changes Aug 10, 2021

View reviewed changes

jacobkeeler merged commit f51f4fd into smartdevicelink:develop Aug 10, 2021

jacobkeeler mentioned this pull request Aug 10, 2021

SDL deletes nested commands and sub menues in a wrong order #3747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sub menues deletion wrong order #3748

Fix sub menues deletion wrong order #3748

AKalinich-Luxoft commented Aug 2, 2021 •

edited

Loading

AKalinich-Luxoft commented Aug 2, 2021

jacobkeeler Aug 4, 2021

AKalinich-Luxoft Aug 7, 2021

jacobkeeler left a comment

jacobkeeler Aug 9, 2021

AKalinich-Luxoft Aug 10, 2021

jacobkeeler Aug 9, 2021

AKalinich-Luxoft Aug 10, 2021

AKalinich-Luxoft commented Aug 10, 2021

jacobkeeler Aug 10, 2021

AKalinich-Luxoft Aug 10, 2021

jacobkeeler Aug 10, 2021

AKalinich-Luxoft Aug 10, 2021

jacobkeeler Aug 10, 2021

AKalinich-Luxoft Aug 10, 2021

		@@ -216,174 +432,219 @@ TEST_F(DeleteSubMenuRequestTest, Run_FindSubMenuFalse_UNSUCCESS) {
		}

		TEST_F(DeleteSubMenuRequestTest, Run_SendHMIRequest_SUCCESS) {

		auto app = application_manager_.application(
		msg_params[strings::app_id].asUInt());

Fix sub menues deletion wrong order #3748

Fix sub menues deletion wrong order #3748

Conversation

AKalinich-Luxoft commented Aug 2, 2021 • edited Loading

Risk

Testing Plan

Reproduction Steps

Summary

CLA

AKalinich-Luxoft commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkeeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AKalinich-Luxoft commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AKalinich-Luxoft commented Aug 2, 2021 •

edited

Loading