test: add more jest tests

[Mohammer5]

Closes DHIS2-14381 (useCheckLockStatus)
Closes DHIS2-14382 (useIsValidSelection)
Closes DHIS2-14383 (mapDataValuesToFormInitialValues)
Closes DHIS2-14384 (buildValidationResult)

Tagging @KaiVandivier as well as @Birkbjo because I did some slight modifications in code you guys wrote

[dhis2-bot]

🚀 Deployed on https://pr-333--dhis2-data-entry.netlify.app

[Birkbjo]

Not sure if this needs it's own file...

[Birkbjo]

Why isn't isOrgUnitLocked good enough? This starts to be a bit too long imo. If it's used for other things other than lockStatus its fine, but if its only for lockstatus, I think we should keep it. It is very descriptive of what it does though.
isOrgunitLocked describes what it's used for, not the implementation details of what makes it locked, which could in theory change, and you would need another function.

[Birkbjo]

Tests looks good, thanks for that!

However I do have some opinions on the refactors, which relates to the refactoring discussion, and our very differing opinions on that subject. But I really don't understand why we have to extract functions into separate files. This is of course a subjective opinion, but having related function in the same file gives a better overview over the related code in my opinion. It does make sense for testing them in isolation though, and you can more easily see that it has a test, so I will give you that. But when the code is only used in a hook for example, it might be enough to just test the output of that hook? Or if you want to test the function, just have multiple describes.
In this case I won't argue that we should move it back, but it's comes back to the old "one file for each function discussion", which I find just bloats the number of files and scatters the code unnecessarily.

Also doing refactors that doesn't really "simplify" the code just irks me the wrong way. Since here you are imposing your version of what you think the code should look like - when the code has already been reviewed and approved by multiple other people. I'm not saying we should never do these small refactors, since it is important to fix "broken windows". But sometimes it's better and less work for everyone to leave the code as is - especially in regards to re-structuring and variable names.

This might be better discussed in a meeting to get input from others as well, since I may totally be in the minority here.

[Mohammer5]

It does make sense for testing them in isolation though, and you can more easily see that it has a test, so I will give you that.

That was the sole reason I moved it. If you feel strongly about it, I can move it back

sometimes it's better and less work for everyone to leave the code as is - especially in regards to re-structuring and variable names.

I disagree with this one for two reasons:

Firstly we're not talking about a lot of code or necessarily a lengthy discussion (unless the discussion is about whether having a discussion is good or not).

Secondly - and I'd say this is the much more important point - variable names are key to understanding the code when reading it. It doesn't mean that my version is better, just because it is newer, that's why we have PR reviews. But I think time is better spent on just asking why I felt changing variable names is an improvement in the first place. Without talking about that, I think this discussion is pointless, so let me try to express my thought process and explain my decisions:

I suppose that this is the name change that has sparked this discussion: isOrgUnitLocked -> isOrgUnitTimeConstraintWithinDataInputPeriodConstraint

I agree that the name is long, and long names aren't "beautiful". I'll quote Martin Fowler a lot, one of the co-authors of the agile manifesto. He and the company he works for (ThoughtWorks) have excellent knowledge about how to work agile, do refactoring, when to do software design, etc.

So first off, what I did here is called "opportunistic refactoring". About when to do it and who should do it, Fowler says: "I prefer to encourage refactoring as an opportunistic activity, done whenever and wherever code needs to cleaned up - by whoever." (source). That's what I did.

So why should we do opportunistic refactoring in the first place? Fowler says: "What this means is that at any time someone sees some code that isn't as clear as it should be, they should take the opportunity to fix it right there and then - or at least within a few minutes. This opportunistic refactoring is often referred to as following the camp site rule - always leave the code behind in a better state than you found it. If everyone on the team is doing this, they make small regular contributions to code base health every day." (same source as above).

So these kinds of changes contribute to a healthier code base; which doesn't mean that my name is necessarily a better one, in this case I'm just arguing about that doing this kind of work should actually be expected of us and we don't do it enough by a big margin. I'm trying to leave code behind better than I found it, even if it's not my code. In that regard Fowler says: "I'm wary of any development practices that cause friction for opportunistic refactoring such as strong CodeOwnership" (same source as above)

So why did I want to change the variable name to something that's uglier than the short name the function had before? I didn't just have a feeling that this could be cleaned up, the function's name actually didn't tell me anything about what it does other than returning a boolean. When I read the code where the function was used, I still had no idea WHY the form should be locked. I had to read through the code and understand what's happening to understand that. And that's not true declarative code. The context of where the function is being used is important.

Before the name change, the code roughly looked like this:

if (isOrgUnitLocked()) {
    setLockStatus(LockedStates.LOCKED_ORGANISATION_UNIT)
}

All the code does say is "If the org unit is locked, then set to lock status to LOCKED_ORG_UNIT". To me this says nothing about what condition needs to occur to cause this locked state. I'd say it's even worse, because the code makes it appear as if an org unit could be locked, which is not the case. The org unit has time constraints and the form could be locked due to the constraints. So the name wasn't just missing information, it was actually misleading.

After I changed the variable name, the code looked liked this:

if (isOrgUnitTimeConstraintWithinDataInputPeriodConstraint()) {
    setLockStatus(LockedStates.LOCKED_ORGANISATION_UNIT)
}

This improves the understandability of the code (imo significantly). I don't need to read though the function "isOrgUnitLocked" to understand the why. This now reads "if the org unit's time constraints are within the data input period's time constraints, then set the lock status to LOCKED_ORG_UNIT". I agree that it takes probably a second or two more to read the name, but it saves at least a few minutes of reading code just to get the understanding that was lacking.

In regards to this kind of practice, Fowler says: "You're looking at some code and you're trying to say, "What does this do? I don't really understand what this does. Let me think about this. Let's burrow in. Ah, now I understand what this code does." Then, before you move on, in the words of Ward Cunningham: "You take the understanding out of your head and you put it into the code,": by restructuring it, by renaming it. Naming is a really important part of all of this. So that the next time you or somebody else comes through that same piece of code, you don't have to go through that puzzle exercise." (source)

our very differing opinions on that subject [...] you are imposing your version of what you think the code should look like

That's precisely why I tagged the people I thought were involved and why I disagree with the term "imposing" as this can't get merged until I get an approval, which I didn't get so far. I even have the following text in the PR's description: "Tagging KaiVandivier as well as Birkbjo because I did some slight modifications in code you guys wrote" because I wanted to make sure you have a change to look at the code. I just realized that it was @tomzemp who wrote the code by digging up the original PR, so tagging you as well.

[The] code has already been reviewed and approved by multiple other people.

After having written my arguments for why I think opportunistic refactoring is appropriate and why I did it in this particular case, I think I already stated why I disagree with this statement. Approvals don't mean that the code that has been merged is perfect as is. There should always be room for improvement and maintaining a code base. Saying that you should "leave the code as is - especially in regards to re-structuring and variable names" is exactly why we have so many projects with messy code bases after they've lived for a long time. We do not take the time to think about the health of a code base. I'd even say that in many cases, developers aren't even aware that this kind of refactoring and the following discussion is a big part of what we have to do ensure maintainable code bases.