Refactoring the maintenance-app

Current situation

State of the DX

When working on an issue of the maintenance app, both development speed and the development experience are awful. It's ridiculously difficult to wrap the mind around what's going on in the code in order to extend or change the code. Refactoring during feature development / bug fixing is nearly impossible as the underlying structure is so complicated that it would take years if we were trying to do this gradually on the side.

State of the app

The code of the different pages (data elements, programs, etc) is more or less decoupled, making it difficult to extract shared logic and simplify the code while working on it. To me it feels like all of these pages could be single apps in terms of their complexity. They have no connection to each other except being pages where you can configure the database / database values.

My approach of refactoring the maintenance app

I don't think that we'd be able to reuse a lot of the existing code as it's a lot of "RxJs black magic" which we're trying to get rid of. So I think we should pick one or two developers (preferably two) who will work on one section (let's say the data element page as it's fairly small). The procedure would be like this:

We lock development on this section for all other developers while refactoring is in progress.
Crucial bugs that appear during the refactoring will have to be fixed by the team doing the refactoring so they know what to keep in mind when writing the new code
The new code will be embedded in the old architecture, but with no respect for existing architecture, it should be built just like we'd do it when writing a new app.
The old code for the section will not be removed, even when when done refactoring, only the Route for the page/section will use a different component (the code coverage is close to 0, so if there's any code using other pages'/sections' code we wouldn't notice until there's a bug; I'd rather get rid of the old code once every page/section has been refactored)
Every page can have it's own redux store, as each page has 100% individual data (if there is some kind of cross over, we still can reuse the reducers, so there's no duplicate code in that regard) => redux stores for each page should be constructed when visiting a page to reduce the amount of data in RAM and the amount of checks in the reducers to a minimum

In order to have no conflict with the existing folder architecture, I'd propose the following folders so we can easily delete the other folders once we're done:

|- src/
|--- components/ - Reusable components
|--- lib/ - so we don't use utils
|--- pages/ - Reactjs route/page entry points
|--- redux/
|----- actions/
|----- epics/
|----- reducers/
|----- stores/ - rootReducers will be created here

Why I'm against writing the app from scratch

When having to maintain 2 code bases for every feature, it'll increase the time of finishing the new app tremendously.
As a developer when working on a project without seeing quick successes / deployments of the work, it's very demotivating
When not getting immediate feedback after finishing one page/section and releasing the change, we'll collect possible bugs and will have to fix them all at the same time we release the new app.

Automated functional test

@varl has mentioned that we want to automate & codify the tests we currently have in JIRA. Another DHIS2 team is trying to get things with cypress to work and there's a cypress plugin to use the Gherkin syntax for feature definitions. This could be a great opportunity to try out these features without having to test too many different things. We could 1. copy the existing tests from JIRA and 2. add missing feature/specs while working on the page/section. If we're doing this, I'll highly suggest that the developers take their time to be developing BDD style, but I wouldn't enforce it. It'd have several advantages though:

Specs don't need to be written after the coding has been done (I guess no one likes writing tests after having finished something)
We'll have specs for every feature (as we'd write a spec before we implement the feature)
As the developer has to think about different scenarios for one feature, it's likely we'll cover more edge cases and the actual coding will be more straight forward

Great initiative, @Mohammer5. I have had success with keeping the old code in place as I rewrite it so that I am able to switch between the implementations. I think it is a helpful technique in complex code bases especially.

While are on the topic of the Maintenance App -- let's also think about if there is a way to avoid backports for it if we are going to refactor it. Else we end up in the situation where one version of DHIS2 has a workable Maintenance app and we still have to support the old one for 2+ years.

While are on the topic of the Maintenance App -- let's also think about if there is a way to avoid backports for it if we are going to refactor it. Else we end up in the situation where one version of DHIS2 has a workable Maintenance app and we still have to support the old one for 2+ years.

That's a good question. We might be able to backport the refactoring of a whole section, I guess. Especially if we don't touch existing code and use new folders like I described, we'd only need to adjust the router of the different branches which shouldn't be too much of a hassle. But it's the maintenance app.. so no idea if I'm right with this judgement

I am very interested in this topic! I believe the maintenance app is far too complicated for its intended function. Simplifying the interface would be a (relatively) easy win for DHIS2 users.

I've been working on some ideas for how the user experience could be better, I'd really like to make this a group activity and get insights from the apps team on how the app could be more effective to use.

Technically speaking, the app shouldn't be so complex (in comparison with some of the beasts in analytics or tracker) as it's intended scope of functionality is rather narrow, so developing a new user interface should be straightforward and include a limited number of different screens to design.

So, just hopping onto this issue to say, I'm in!

I am very interested in this topic! I believe the maintenance app is far too complicated for its intended function. Simplifying the interface would be a (relatively) easy win for DHIS2 users.

@joeDHIS I couldn't agree more that simplifying the maintenance app would be great! But I think we need to discuss whether we want to do this at the same time. It will increase the time of refactoring and just copying the functionality with be very demanding as the devs will have to go through the code and find the nuances, how stuff behaves, e. g. changes in some parts of the form can change other values of the very same form.

@joeDHIS (CC: @varl) If we go the way I described withe cypress/gherkin specs, ideally you can write specs in the gherkin language already in order to define new features/behavior and the devs could use those specs right away. They might have to adjust the way the specs were written (formally, the content will stay the same) and ideally give feedback to you so you know how to improve your gherkin skills (the first specs always need some degree of revision)

I've worked on the maintenance-app quite a bit, so I'm quite familiar with its problems and the specific challenges we will face when refactoring. I'll start describing these and from there try to explain how I would like to refactor things.

1. Problems and challenges

The app was initially developed alongside of d2 and d2-ui, and in some parts feels like it is developed as a visual layer over d2. Unfortunately, the reality is much more complex than that, and in places where this has proved to be the case various techniques for managing state have been applied. The end result is that there is no single notion of an app-state and this is problematic.
The app seems to be developed with the initial notion that all sections can be assumed to be equal. Unfortunately this is not the case, and more and more conditions were added to existing components to mitigate this. Obviously, after 5 years, this leads to a big tangled mess of spaghetti.
At the same time, it is true that many sections are extremely similar, they are just a simple CRUD interface for a single metadata object. The only things that differ per section for these types of sections are[*]:
- Model name
- Fields to show in list (i.e. table columns)
- Filters fields to above the list
- Fields to show in add/edit form
- Potentially some interdependencies between form fields
On the other side of the spectrum, there are sections like the program section, which is using a stepper form, uses a couple of different d2-models, and has its own state. Or the locales section which looks fairly standard but is actually not a metadata object, so is interacting with the web-api in a completely different way. I'm sure there are more/better examples, but the main point is: some sections deviate a lot from the standard scenario.

[*] @Birkbjo: I am deliberately not mentioning models with custom ContextMenuActions here because quite often these are then routing to a custom view or doing other funky stuff._

2. Some refactoring ideas

We need a proper app state for the app as a whole and each individual section. I would prefer redux + thunks (and possibly some cancellation middleware), but I'd be happy just to have any singular state management solution throughout.
Because there are so many sections, and many of them are very similar, we should not manually write the state (actions/reducers) for each section, but instead build something that can generate the state for a default-section (i.e. default as described in 1.3). For more complex sections we should manually write the section state.
Because I think the app's design was not optimal at a basic level, I think this will be more of a full rewrite than a refactor. Saying that though:
- It would be nice if we could have the old and new implementation running side-by-side, so it's easy to verify that the new implementation has equivalent behavior as the old.
- It would be nice if we could still somehow introduce new features gradually. To achieve this we could possibly just create a v2 folder and put all the new stuff in there, and plug new features into v2 by leveraging the router...

To avoid backports we could do this: Create a feature-support.js like this:

import { getSystemVersion } from './api'

export const AMAZING_FEATURE = 'AMAZING_FEATURE'

const features = new Map([
    [ AMAZING_FEATURE, { introduced: 29, removed: 33 }, ]
])

export default function supports(name) {
    const { introduced, removed } = features.get(name)
    const { minor } = getSystemVersion()
    return introduced <= minor && (!removed || minor < removed)
}

And use this helper to conditionally handle things

import supports, { AMAZING_FEATURE } from './feature-support'

if (supports(AMAZING_FEATURE)) {
    // do amazing stuff
} else {
    // do slightly less amazing stuff (but still impressive of course)
}

Simplest way to work would be to prepare all state in the normal way, in createStore. But it could be that with so many sections the app starts up slow. To combat this, we could dynamically add reducers to the store when a user accesses a section route. This technique is used by Twitter and explained in this article.

I'm sorry that I've been late to the party on this one. I think @HendrikThePendric touches on a lot of the problems and challenges. However I've always believed that the most challenging parts are handling the overall app state and the interconnection to forms. This is where all the complexity boils down to. Many of the objects/models are simple, without embedded objects, and in those cases it's handled pretty simply today (apart from mutation and the resulting pitfalls). However, we need to design for the complex use cases, as those are the ones that have evolved into the mess that it is today.

Forms are so integral to the app, that I believe we should work out unified form handling (#25) first, especially the role of ui-forms. While implementing this, we could also use a section in Maintenance as a prototype. Speaking of forms, I believe something we haven't really discussed is a way to implement 'rules', so we have an easy way to eg. show and hide fields depending on the form-state.

Because there are so many sections, and many of them are very similar, we should not manually write the state (actions/reducers) for each section

I agree, and I think we've both kind of had this in our mind and thought about implementations. I've at least played with normalization of schemas here: https://github.com/dhis2/maintenance-app/commit/6f89ddd43bea79a83a74cae521952ae98e6dd04c . This is a crucial part to get right.

Every page can have it's own redux store, as each page has 100% individual data (if there is some kind of cross over, we still can reuse the reducers, so there's no duplicate code in that regard)

I'm unsure about this actually. We could share a lot of the information between models/objects if we went with one normalized store. It also goes against the 'redux mantra' of having one store, quote from redux docs:

"it is possible to create multiple distinct Redux stores in a page, but the intended pattern is to have only a single store. Having a single store enables using the Redux DevTools, makes persisting and rehydrating data simpler, and simplifies the subscription logic."

In my mind I've thought we could have one 'models' state, where we store the actual data for the objects. Something like this: { models: { dataElements: { byId: { someId: {}, }, }, }, } Here, we could give a component just the slices of the data it needs, creating a data-dependency spec in the process.

The old code for the section will not be removed, even when when done refactoring, only the Route for the page/section will use a different component

I think this would be the approach to go with. Related to Routes: the app is still using react-router 3. Which isn't a big problem in it self, but we should look into updating this. It may not be too trivial, as some of the data-loading is handled in onEnter routes, and you have weird stuff like this https://github.com/dhis2/maintenance-app/blob/master/src/App/App.component.js#L85. It also touches on the entire app, so refactoring this is quite risky.

Another tricky problem to solve is where we should draw the line of 'default'-handling and overrides; if we should generate forms in the simple cases, or if we should have a component for each of the forms in the app. Put it simply: should we manually add all the fields in jsx, or should we have logic to generate fields based on schemas?

Overall, I think if we solve the interconnection of the forms and state, we've come a long way.

In my mind I've thought we could have one 'models' state, where we store the actual data for the objects. Something like this: { models: { dataElements: { byId: { someId: {}, }, }, }, } Here, we could give a component just the slices of the data it needs, creating a data-dependency spec in the process.

That sounds like a nice way to go to me.

Another tricky problem to solve is where we should draw the line of 'default'-handling and overrides; if we should generate forms in the simple cases, or if we should have a component for each of the forms in the app. Put it simply: should we manually add all the fields in jsx, or should we have logic to generate fields based on schemas?

I think a large part of what I don't like about the maintenance-app, d2, d2-ui and related legacy code are the abstractions. Abstractions done well can be helpful, but to me a lot of the abstractions I've encountered in these apps are extremely convoluted and not that helpful.

In general I think I would try starting out without any premeditated abstractions, just doing things manually and then after a while see if anything warrants abstracting. I can imagine that trying to decide too far in advance would be difficult, given how complicated it seems to be to abstract common logic for this app.

There is so much valuable information in the schemas, that it would almost be a crime not to leverage that.

Personally, I think that we should still generate a lot of things (so via abstractions) but we should probably take the opposite approach as is being done now: If no field/form component is specified we fallback to some abstraction that generates fields/ entire form based on the resource schema.

I think that we should be careful in how we assess the optimal way forward. It is important that we consider current problems when deciding on the new architecture, as we are doing now. However, because we are continuously fixing stuff in sections that don't work well, we might forget to consider the parts of the current implementation that do work well. There are 44 sections in the maintenance app, and I would guesstimate that at least half of these are just simple CRUD interfaces without any subtleties that are not captured by the info in schema. Creating an individual form for all of these seems like a bad plan to me.

Can be closed, we've reached a conclusion and workflow & started working on it already

dhis2 / notes