FritzAndFriends / ResourceManagement

A resource management application originally designed for Sebastian Riding Associates.
MIT License
35 stars 28 forks source link

#56 - Refactoring Blazor code into ViewModels #58

Closed SimonGeering closed 5 years ago

SimonGeering commented 5 years ago

This PR is to refactored all code blocks the Blazor views out into corresponding ViewModel classes to improve separation of presentation markup from presentation logic.

NOTE: I was unable to get the app running due to URL routing/nav issues

As such the limit of dev testing for this PR amounts to:

  1. A build of the solution after each refactoring.
  2. Running of existing unit tests
  3. Running solution to the main page and checking for client-side errors

I would recommend that this is thoroughly reviewed before being merged, preferably by pilling this branch locally so it can be loaded/debugged.

Details of changes View models have been placed in a ViewModel folder in the main Fritz.ResourceManagement.WebClient project. Should we subsequently find that this causes problems with the need to reference this from the Unit Test project we have the option to extract the ViewModel classes into their own .Net Standard Lib.

I have added "//TODO: Simon G - " task comments to areas of note where there are options for further improvement or refactoring, or where I have questions or concerns about how the code might work in a ViewModel.

The registration of ViewModels in DI has been pulled out into an extension method to help segregate ViewModel DI as a sperate distinct set of registrations. I have not extracted interfaces for DI as I don't believe it is currently required, this may change if testing of UI collaborations becomes complex.

Thoughts on further enhancements There are two main areas where code is left in the Blazor Code blocks:

  1. Overrides to Blazor protected ComponentBase members It may be preferable to also move this code out of the markup. If this is the case then it should be possible to derive the component from a CodeBehind base class as noted in the Blazor documentation which would still allow the DI of the Model
  2. Component Parameter properties - [param] properties I have attempted to delegate the param properties to the corresponding VM members. It may be preferable to review each usage of pram properties to see if passing the full ViewModel of the component in as param rather than injecting it would be more desirable.
SimonGeering commented 5 years ago

@csharpfritz please let me know if you are happy with the nature of this change and the current limitations of the scope of dev testing? If so I will convert this from a draft to a PR.

SimonGeering commented 5 years ago

FYI - This was compiling and running test for me locally [It works on my machine lol πŸ˜πŸ‘]. Seriously though, it looks like The Azure Build is failing because it is using 3.0.100-preview5-011568 SDK to build Preview 7 code. Probably need to update the SDK on the MS host agent in a pre-build task.

csharpfritz commented 5 years ago

I've updated the build process to use Preview 7.

There is now a unit test failure baked in for the ExpandSchedule operation that needs to be completed.

SimonGeering commented 5 years ago

I would recommend switching the CI/CD to Release build configuration and wrapping those sorts of "TODO" unit test in a #if DEBUG block. As it stands now, IMHO, there is more value in completing the PR and adding an issue to address the functional shortcomings pertaining to the unit test in a separate item of work.

An alternative might be an assertion that caused a warning message rather than an exception.

As it stands I assume the failing test is causing PR policy to block the merge?

csharpfritz commented 5 years ago

@SimonGeering Done... I've added the conditional compilation statement

SimonGeering commented 5 years ago

Looking like I don't have permission to re-run the CI build.

csharpfritz commented 5 years ago

I re-ran it, and it was successful.. just didn't attach the results to the PR

Jeff

On Tue, Jul 30, 2019 at 12:53 PM Simon Geering notifications@github.com wrote:

Looking like I don't have permission to re-run the CI build.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FritzAndFriends/ResourceManagement/pull/58?email_source=notifications&email_token=AAATF4N3SF7J4DUOVGI3QK3QCBWYXA5CNFSM4IHNT4FKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ETOVY#issuecomment-516503383, or mute the thread https://github.com/notifications/unsubscribe-auth/AAATF4KPID2AHFFLUJPT2MDQCBWYXANCNFSM4IHNT4FA .

csharpfritz commented 5 years ago

Thank you for all the work on this PR. A big help!!