[SDL-0255] Enhance BodyInformation vehicle data #354

ymalovanyi · 2020-12-01T10:36:06Z

Fixes #338

Risk

This PR makes minor API changes.

Testing Plan

I have verified that I have not introduced new warnings in this PR
I have run the unit tests with this PR
I have tested this PR against Core and verified behavior

Unit Tests

Added unit tests cover [SDL-0255] Enhance BodyInformation vehicle data changes

Core Tests

each of the properties in the struct DoorStatus can be received from HMI
each of the values in the enum DoorStatusType can be received from HMI
each of the properties in the struct GateStatus can be received from HMI
each of the properties in the struct RoofStatus can be received from HMI
doorStatuses, gateStatuses, roofStatuses properties are present in BodyInformation struct
GetVehicleData response message contains bodyInformation parameter with doorStatuses, gateStatuses, roofStatuses properties

Core version / branch / commit hash / module tested against: smartdevicelink/sdl_core#3584
HMI name / version / branch / commit hash / module tested against: smartdevicelink/sdl_hmi#466

Summary

Applied [SDL-0255] Enhance BodyInformation vehicle data changes

Changelog

Breaking Changes

N/A

Enhancements

New structs DoorStatus, GateStatus, RoofStatus
New enum DoorStatusType
Added required getters/setters in related RPC classes

Bug Fixes

N/A

Tasks Remaining:

N/A

CLA

I have signed the CLA

ymalovanyi · 2020-12-01T11:57:44Z

@santhanamk the PR is ready for Ford review.

santhanamk

Hi @ymalovanyi I reviewed this PR. I have a few suggestions on the unit tests. Please see if anything needs to be changed.

santhanamk · 2020-12-07T21:39:16Z

tests/Test.js

+];
+
+const JSON_DOORSTATUS = Test.JSON_DOORSTATUS = {
+    [DoorStatus.KEY_LOCATION]: Test.JSON_GRID,


@ymalovanyi On line 442 does Test.JSON_GRID need to be changed to Test.GENERAL_GRID?

No, because then test will fall, as Grid is a struct. Similarly, as in JSON_WINDOWSTATUS, we have Test.JSON_GRID.

santhanamk · 2020-12-07T21:46:23Z

tests/Test.js

+];
+
+const JSON_GATESTATUS = Test.JSON_GATESTATUS = {
+    [GateStatus.KEY_LOCATION]: Test.JSON_GRID,


@ymalovanyi On line 459 does Test.JSON_GRID need to be changed to Test.GENERAL_GRID?

Same as in above comment.

santhanamk · 2020-12-07T23:31:40Z

tests/node/rpc/structs/BodyInformationTests.js

+                [BodyInformation.KEY_PASSENGER_DOOR_AJAR]: Test.GENERAL_BOOLEAN,
+                [BodyInformation.KEY_REAR_LEFT_DOOR_AJAR]: Test.GENERAL_BOOLEAN,
+                [BodyInformation.KEY_REAR_RIGHT_DOOR_AJAR]: Test.GENERAL_BOOLEAN,
+                [BodyInformation.KEY_DOOR_STATUSES]: Test.JSON_DOORSTATUS_LIST,


@ymalovanyi on line 33 does Test.JSON_DOORSTATUS_LIST need to be changed to Test.GENERAL_DOORSTATUS_LIST?

No, because then test will fall, as DoorStatus is a struct. That is why we need to use list of JSON's.

santhanamk · 2020-12-07T23:32:31Z

tests/node/rpc/structs/BodyInformationTests.js

+                [BodyInformation.KEY_REAR_LEFT_DOOR_AJAR]: Test.GENERAL_BOOLEAN,
+                [BodyInformation.KEY_REAR_RIGHT_DOOR_AJAR]: Test.GENERAL_BOOLEAN,
+                [BodyInformation.KEY_DOOR_STATUSES]: Test.JSON_DOORSTATUS_LIST,
+                [BodyInformation.KEY_GATE_STATUSES]: Test.JSON_GATESTATUS_LIST,


@ymalovanyi on line 34 does Test.JSON_GATESTATUS_LIST need to be changed to Test.GENERAL_GATESTATUS_LIST?

Same as in comment above, GateStatus is a struct.

santhanamk · 2020-12-07T23:33:15Z

tests/node/rpc/structs/BodyInformationTests.js

+                [BodyInformation.KEY_REAR_RIGHT_DOOR_AJAR]: Test.GENERAL_BOOLEAN,
+                [BodyInformation.KEY_DOOR_STATUSES]: Test.JSON_DOORSTATUS_LIST,
+                [BodyInformation.KEY_GATE_STATUSES]: Test.JSON_GATESTATUS_LIST,
+                [BodyInformation.KEY_ROOF_STATUSES]: Test.JSON_ROOFSTATUS_LIST,


@ymalovanyi on line 35 does Test.JSON_ROOFSTATUS_LIST need to be changed to Test.GENERAL_ROOFSTATUS_LIST?

Same as in comment above, RoofStatus is a struct.

santhanamk · 2020-12-07T23:34:36Z

tests/node/rpc/structs/DoorStatusTests.js

+
+        this.getExpectedParameters = function (sdlVersion) {
+            return {
+                [DoorStatus.KEY_LOCATION]: Test.JSON_GRID,


@ymalovanyi on line 18 does Test.JSON_GRID need to be changed to Test.GENERAL_GRID?

No, because then test will fail. We're checking for parameters here, and Grid is a struct. That is why we are passing JSON parameters of the struct, but not the whole struct itself.

santhanamk · 2020-12-07T23:35:26Z

tests/node/rpc/structs/GateStatusTests.js

+
+        this.getExpectedParameters = function (sdlVersion) {
+            return {
+                [GateStatus.KEY_LOCATION]: Test.JSON_GRID,


@ymalovanyi On line 18 does Test.JSON_GRID need to be changed to Test.GENERAL_GRID?

Same as in above comment.

santhanamk · 2020-12-07T23:36:20Z

tests/node/rpc/structs/RoofStatusTests.js

+
+        this.getExpectedParameters = function (sdlVersion) {
+            return {
+                [RoofStatus.KEY_LOCATION]: Test.JSON_GRID,


@ymalovanyi On line 19 does Test.JSON_GRID need to be changed to Test.GENERAL_GRID?

Same as in above comment.

santhanamk · 2020-12-07T23:37:00Z

tests/node/rpc/structs/RoofStatusTests.js

+            return {
+                [RoofStatus.KEY_LOCATION]: Test.JSON_GRID,
+                [RoofStatus.KEY_STATUS]: Test.GENERAL_DOORSTATUSTYPE,
+                [RoofStatus.KEY_STATE]: Test.JSON_WINDOW_STATE,


@ymalovanyi On line 21 does Test.JSON_WINDOW_STATE need to be changed to Test.GENERAL_WINDOW_STATE?

No, because WindowState is a struct.

santhanamk

@ymalovanyi thank you for the clarification. I am approving this PR.

ymalovanyi · 2020-12-09T09:56:14Z

@santhanamk, thank you for the approval! @crokita, Ford approved this PR, please review.

jordynmackool · 2020-12-10T20:37:53Z

Hi @ymalovanyi and @santhanamk, since the core PR for this feature is not complete, it seems that this PR would have not been tested fully. To ensure the review process goes smoothly:

Please avoid removing sections from the PR template as all the information requested is needed.
Please provide the Core branch/PR that this was tested against.

Once this PR is fully tested, tag me and Livio will review. Thank you!

vladmu · 2020-12-18T17:34:43Z

@jordynmackool @crokita the description was fixed to include all sections from the template and the code was tested against the Core and HMI (corresponded links also included in the description). Please review.

…nhance-BodyInformation-vehicle-data # Conflicts: # tests/Test.js

vladmu · 2021-01-15T16:36:49Z

@crokita, the PR was updated with the latest develop, please don't merge this PR after the approval before this related rpc_spec PR smartdevicelink/rpc_spec#297 is not merged.

renonick87

This PR is approved. Waiting on smartdevicelink/rpc_spec#297 to be approved before merging.

[SDL-0255] Enhance BodyInformation vehicle data

3e2b9c7

santhanamk reviewed Dec 7, 2020

View reviewed changes

santhanamk approved these changes Dec 8, 2020

View reviewed changes

ymalovanyi marked this pull request as ready for review December 9, 2020 09:55

crokita mentioned this pull request Jan 15, 2021

Feature/0296 possibility to update video streaming capabilities #302

Merged

4 tasks

Merge remote-tracking branch 'origin/develop' into feature/SDL-0255-E…

bea495e

…nhance-BodyInformation-vehicle-data # Conflicts: # tests/Test.js

renonick87 approved these changes Jan 25, 2021

View reviewed changes

renonick87 merged commit 0c79267 into smartdevicelink:develop Jan 26, 2021

[SDL-0255] Enhance BodyInformation vehicle data #354

[SDL-0255] Enhance BodyInformation vehicle data #354

Uh oh!

Conversation

ymalovanyi commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

Tasks Remaining:

CLA

Uh oh!

ymalovanyi commented Dec 1, 2020

Uh oh!

santhanamk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymalovanyi Dec 8, 2020 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santhanamk left a comment

Choose a reason for hiding this comment

Uh oh!

ymalovanyi commented Dec 9, 2020

Uh oh!

jordynmackool commented Dec 10, 2020

Uh oh!

vladmu commented Dec 18, 2020

Uh oh!

vladmu commented Jan 15, 2021

Uh oh!

renonick87 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ymalovanyi commented Dec 1, 2020 •

edited

Loading

ymalovanyi Dec 8, 2020 •

edited

Loading