Device manager: User session details screen (PSG-685) #6693

[paleksandrs]

Closes #6693

[github-actions]

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/kjC2BK

[gileluard]

@paleksandrs could you please add some comments and ideally some screenshots to give me more context to this PR?

[paleksandrs]

@gileluard this ticket is the implementation of a session details screen that looks like this:

Light	Dark

ACs:

• Includes section ‘Session’ with fields session name, session id, and last activity
• Includes section ‘Device’ with field ‘IP address’
• ios/android - data copied to clipboard on long press
• If any piece of information is missing or empty, the corresponding row is hidden
• If an entire section has no visible rows, it is hidden as well

[Johennes]

Kudos for adding tests from the start! 👌

[gileluard]

Sorry but I wanted to take all the time needed to have a clear and fair review. Hopefully, this will be helpful.

As a general advice, it seems that you added some files (at least UserSessionFlowCoordinator) using xcode which may lead to some build error as we use xcodegen to regenerate the Riot project. You should rather run xcodegen adn then pod update more often to make sure the CI will build successfully the project.

From the architecture standpoint, the way you organize the coordinators confused me a bit as you tried to mix flow coordinator and screen coordinators. You may benefit from reorganizing your coordinators this way:
UserSessionsFlowCoordinator -> this coordinator handles the entire flow. It manages the navigation router and push screen coordinators
|
-- UserSessionsListCoordinator -> this coordinator handles the sessions list screen
|
-- UserSessionDetailCoordinator -> this coordinator handles the session Detail screen
This way you may plenty use our template and have the folder hierarchy with consistent name between coordinators, view models, and views:
UserSessionsFlow -> mainly contains the UserSessionsFlowCoordinator and the UserSessionsFlowCoordinatorBridgePresenter files
|
-- UserSessionsList
|
-- UserSessionDetail

You can take a look at the RoomAccessCoordinator as an example.

From the UX standpoint, the detail screen is expected to display more information then the list view and I cannot see any information about the session verification state. If the design is definitive, it could be interesting to talk about this topic with the designer.

[Johennes]

From the UX standpoint, the detail screen is expected to display more information then the list view and I cannot see any information about the session verification state. If the design is definitive, it could be interesting to talk about this topic with the designer.

This is probably impossible to understand from this PR alone but there will be an intermediate "session overview" screen inserted between the list view and this "session details" screen in a future PR. We just started on this screen first to minimize conflicts with the work Stève had ongoing at the time.

Also, the session details screen itself will be enhanced with further rows in yet another pull request once we have parsed the additional data out of the user agent.

[paleksandrs]

Thanks for the detailed code review and spending time on it.

Yes, indeed I added some files using Xcode. Sorry, I haven't worked with xcodegen before, how should the files be added correctly then?

About the architecture. I am a little bit confused how the coordinator pattern is being used in the project. I might be wrong but from what I know coordinators are usually responsible for creating and starting the flow on the screen, that is their main responsibility. The application can have many flows and each flow can use the same views.

As you mentioned that there are 2 types of coordinators - "flow coordinator" and "screen coordinator", but they both have one protocol and main function start (that should start the flow on the screen). And they both are named the same in the project that makes it even harder to distinguish. If I look in the project existing "screen coordinators" implementation usually sets delegate or callback in the start method, that could be done in any other method (e.g. init method). start method should start the flow on the screen. Most important existing "screen coordinator" implementation in most cases just creates the view and view model (that could be done by for example builder pattern) and simply re-delegates all actions back to "flow coordinator" (e.g. UserSessionsOverviewCoordinator, RoomAccessTypeChooserCoordinator, RoomUpgradeCoordinator, MatrixItemChooserCoordinator, etc.), so it is hard to understand what is the purpose of "screen coordinator" in general. Would you mind explaining this, please?

Because of that, I created UserSessionFlowCoordinator that will handle flow of session overview on the session details screen. So now, when you call start on UserSessionFlowCoordinator it will display the session overview screen and will navigate to the session details screen as well. I think this particular flow will be used elsewhere as well so that's why it has a dedicated "flow coordinator".

[gileluard]

Let's consider my general comment. UserSessionsFlowCoordinator is the only needed flow coordinator and UserSessionsListCoordinator, UserSessionDetailCoordinator will be the screen coordinators handled by UserSessionsFlowCoordinator.

UserSessionsFlowCoordinator.start() will create the UserSessionsListCoordinator and add it to the navigation router.

UserSessionsListCoordinator.start() and UserSessionsFlowCoordinator.start() will basically define the view model completion block.

When the user taps an item in the screen handled by the UserSessionsListCoordinator, the UserSessionsListCoordinator should call the completion block with the appropriate parameters so the UserSessionsFlowCoordinator will instantiate a UserSessionDetailCoordinator with the appropriate UserSessionInfo and push it to the navigation router.

RoomAccessCoordinator is a very simple of what a flow coordinator should be. You may also duplicate the pushScreen() method if you need.

Feel free to DM me if you need more information.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[paleksandrs]

@gileluard thank you for your comments, I fixed most of them. I wasn't aware of the folder structure that is being used in the project, so hopefully, you should be able to run the tests now. I also change the architecture to use "screen coordinator" to match other places in the app. Please let me know if you have any other comments.

[codecov]

Codecov Report

Base: 9.92% // Head: 10.04% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (4bd3979) compared to base (75bd23d).
Patch coverage: 56.32% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6750      +/-   ##
===========================================
+ Coverage     9.92%   10.04%   +0.11%     
===========================================
  Files         1484     1490       +6     
  Lines       150486   150805     +319     
  Branches     60742    60858     +116     
===========================================
+ Hits         14934    15143     +209     
- Misses      135006   135108     +102     
- Partials       546      554       +8     

Impacted Files	Coverage Δ
...ls/Coordinator/UserSessionDetailsCoordinator.swift	0.00% <0.00%> (ø)
...Flow/Coordinator/UserSessionsFlowCoordinator.swift	0.00% <0.00%> (ø)
.../Coordinator/UserSessionsOverviewCoordinator.swift	0.00% <0.00%> (ø)
...ervice/MatrixSDK/UserSessionsOverviewService.swift	0.00% <0.00%> (ø)
...erSessionsOverview/View/UserSessionsOverview.swift	0.00% <ø> (ø)
.../UserSessionDetails/UserSessionDetailsModels.swift	36.36% <36.36%> (ø)
...erSessionDetails/View/UserSessionDetailsItem.swift	50.94% <50.94%> (ø)
...s/UserSessionDetails/View/UserSessionDetails.swift	85.71% <85.71%> (ø)
...ionDetails/MockUserSessionDetailsScreenState.swift	100.00% <100.00%> (ø)
...erSessionDetails/UserSessionDetailsViewModel.swift	100.00% <100.00%> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gileluard]

Great job ! Seems we have a brand new screen within a great architecture :)