Inherit from hmpo-date-controller #39
Conversation
Changes the base controller to inherit from the new hmpo-date-controller module instead of hmpo-form-wizard's controller. This provides the base controller with support for dates input via hmpo-template-mixin's 'input-date' mixin. Thus making the date-controller redundant. We should consider removing the date controller in a future release.
|
💃 💃 LGTM 💃 💃
|
|
My hope is that hmpo-date-controller will be merged into hmpo-form-controller at some point so it shouldn't be written in ES6 unless hmpo-form-controller is converted to ES6. |
|
Fair. |
|
I'm happy with this could @JoeChapman or @joefitter please give this look 👓 👀 |
|
See also: UKHomeOffice/passports-date-controller#1 And any other issues that get raised on the repo in case they are things that people decide should be addressed prior to use in HOF. |
|
UKHomeOffice/passports-date-controller#3 should address the only open issue on the new repo. |
| var util = require('util'); | ||
| var _ = require('lodash'); | ||
|
|
||
| // In HOF we are more lenient with date inputs than in vanilla | ||
| // hmpo-form-wizard and so must override some validators |
There was a problem hiding this comment.
Can you remove the comments please?
There was a problem hiding this comment.
Happy to remove if others agree. My rationale was that it's a strange thing to do and so needed some explanation.
|
I'm not sure how I feel about extending base-controller from a date controller. It feels a bit backwards. |
|
Does this also change the way one would configure their fields for a date? |
It'd be the hof base controller vs the hmpo-date-controller though to it's not tooo bad. |
|
The idea is that this stuff should really be supported out of the box. I'd like to get this functionality in the hmpo-form-controller if I can convince @gavboulton that this module is solid. i.e. "Date controllers" should never have been a thing. @JoeChapman Yes is changes that. (i.e. Simplifies it.) It is explained in the README for the new repo. |
Could hmpo-date-controller not be mixed into hmpo-form-controller, as opposed to extending from it? Otherwise we end up with a long chain of classes, where the functionality becomes broader, but the nomenclature becomes more and more specific, which doesn't suit the current architecture, i.e., HOF extends from HMPO. |
|
I considered calling it "hmpo-date-base-controller" but it felt ugly. - Naming things is hard. Re: Why is it a separate module, see HMPO/hmpo-form-wizard#35 |
|
Yeah, I think a separate module is a good idea, but I still wonder if it might be better if HMPO form controller require'd HMPO date controller and mix(ed)in it's functionality. That way, HOF-base controller could treat HMPO form/date controllers as the same entity, without concern for what particular class extends which class.
^ yeah, agreed, a bit ugly, but although they are separate entities, I don't think they should be thought of as such by HOF. |
|
@daniel-ac-martin the read me in HMPO date controller shows how include a date field in a step definition, but is the way a date field is composed different to how it once was, I thought it too was changing? |
Are we changing the input-date mixin then? Do you have a link to the PR? As long as we pin to compatible versions of hmpo-date-controller and hmpo-template-mixins we should be fine. |
Not necessarily, however, I thought we might. Are date fields configuration unchanged? |
|
@daniel-ac-martin @easternbloc @JoeChapman Just having another look at this PR - this does feel like a very strange way of doing things, I don't think our HOF base controller should extend a date-controller, very few of our steps actually require date functionality, and the ones that do use the HOF date controller. I would suggest leaving the base controller to extend form-wizard controller as it currently does, and mixin the date functionality to the hof date controller. We would end up with something like: |
|
I tend to agree. Ideally, hmpo-date-controller would be a part of hmpo-form-controller/hmpo-wizard(-)controller and would just be included for free, as per @daniel-ac-martin 's earlier comment. However, if that can't be done, I would ask that we mix in the date functionality or replace the hof-date-controller with the hmpo-date-controller. I.e, hmpo-date-controller extends hof-base-controller, as we currently do with hof-controller. |
|
@joefitter, @JoeChapman:The argument to eventually get the functionality into hmpo-form-controller is that this functionality should be provided out of the box. i.e. "Date controllers" should not be a thing! If we retain HOF's date-controller we will be making the opposite argument. Also, whilst we would be providing a better date controller that neither requires the programmer to extend it nor to provide it with the date component fields in the options / step definition, we would still be requiring the programmer to think about whether or not they need date functionality in a particular step. I would argue that is a small 'gotcha'. The 'input-date' mixin should just work. |
|
Another important issue is this: UKHomeOffice/passports-date-controller#4 |
|
@daniel-ac-martin @JoeChapman OK I have changed my view on this slightly from looking more at what this controller is actually achieving. IMO this logic should be tied to the input-date mixin. I think we should stop thinking of things as separate controller, separate view etc, but instead be packaging up fully functional components. I believe these should live within hof as a set of components, each one with their repo etc. This example would be hof-component-date or similar, and contain the rendering logic (3 inputs, all meta etc), as well as the process logic to save the processed date to the model. The developer would simply use the input-date mixin and all the boilerplate including error messages etc would be hidden, much like they are for input-text etc. I would suggest these would be a superset of template mixins, with any hof components taking precedence. |
Feels like a nice idea to me. Moving into a more composable state seems like a more scalable approach. 👍 |
|
After further thought I would like to reinforce my last comment to say that this PR should not be merged until UKHomeOffice/passports-date-controller#4 is addressed, as it makes it possible for a malicious user to break the form until the instance is restarted. As for how we structure the modules (see the last two comments) I think that work should be done at a later date and should not block this going in. (Once the issue I mentioned is properly addressed of course!) |
If we are going to address components at a later date, then why is it better that this controller goes in at this point and not at a hof-controller level? |
|
@JoeChapman @daniel-ac-martin I agree that having something called "date-controller" and our "base-controller" extending from it is not the way to go, it just seems wrong. We have a date-controller which extends base-controller in this lib and we use this for steps which include a date - can't we apply your fix/change here? If we apply it further up the architecture, and extend base controller from it, then doesn't that defy the point of us having a hof-controllerS lib, it would just be a controller that inherits everything, including confirm page and any other functionality - aka a framework |
|
While we wait to implement a "component" like architecture, please can the date-controller be worked into the HOF-controllers? |
|
Sounds like we can now get this functionality into the form-controller itself. |
|
@daniel-ac-martin I thought you were just talking about the template mixins? |
|
@daniel-ac-martin I'm still not convinced the date functionality should be core to the form controller. IMHumbleO, the date functionality is a feature, not an explicit requirement of any form request handler and should therefore be an opt in feature. Just my two cents, anyone else, @easternbloc @gavboulton @joefitter |
|
The functionality is for formatting/validating dates so we wouldn't be able to handle it just in template mixins. Ideally we would package it up into a component, but in the meantime can't we use the date-controller in hof-controllers? |
|
@gavboulton: No I mean support for processing the data that comes from someone filling in input-date mixin from template mixins. |
|
@JoeChapman If we are offering an In the interim I have no problem using the hof date controller, but I really think we shouldn't have to use a custom controller just for a date - it should be handled by the component itself. But not mixed in to every controller. |
|
@joefitter: Just wanted to point out that the mixin isn't new. It's been there ages without any support for actually processing it. IMO it's basically a massive 'gotcha' for any dev new to this stack. Regarding components, I have a few ideas. One is to have a new concept to sit alongside formatters and validators probably called 'processors' (unless someone can think of a better name). These would have the power to amalgamate request body parameters prior to being validated. (Which is basically how my current implementation works.) The other idea, which is a long shot, would be to change the input-date mixin itself. That might mean that we don't need any of this and instead can do everything with a validator. I need to experiment to see whether this is feasible. |
|
I know and I agree, the input-date mixin is not useful out of the box, and that is why I think it should be removed. To replace it we would have an input-date component which would consist of a template mixin, processing/formatting/validation packaged within a middleware which then hands off the formatted input-date field to the standard controller |
|
we are going to have a bunch more of these - postcode-lookup, file upload etc etc, if the processing/validation etc for all of these is added to the base controller it will get unmaintainable very quickly, which is why we were looking at components |
|
@joefitter: I agree. In the meantime though, I think we should get input-date working in the base packages and take it out when we are ready to change the architecture. It's better to have a complete implementation in there that can be taken out as one, rather than an incomplete one. (The form controller already has validators that look like they are designed for input-date.) |
|
@daniel-ac-martin @joefitter I agree with Dan's last point. It needs to work. We don't have the capacity to build components immediately. So long as this is chalked up as tech debt in the appropriate repos and we continue to work at a solution when we have the appropriate resource, which is soon. |
|
@gavboulton @JoeChapman @easternbloc how do you feel about this? I guess its up to the HMPO guys whether or not they want this merged into the base controller. Also if it is working now is there any sense on changing it for an imperfect solution? Why don't we carry on as we have been until a better solution can be worked on? |
Is it working? |
|
We use hof date controller in GRO which works fine |
|
No. The solution is incomplete. We are missing the bit that takes the data in req.body and gets it through to the validators. |
|
@daniel-ac-martin that sounds straight-forward, is it? If so, could it be transplanted into hof date controller instead? |
|
Yes you can use a special controller, but that seems crazy to me just for date support. Also some forms will need to run using their own custom controller on all steps. How are they supposed to handle dates? Should they maintain two custom controllers, identical but with one inheriting from HOF's date controller instead of HOF's base controller. |
|
@JoeChapman It could go into HOF's special date controller but you've lost a lot of the benefit in doing so. |
|
My preference would be the hmpo-form-controller, followed by hmpo-form-wizard's controller, followed by HOF's base controller followed by poking my eyes out with a blunt fork, followed by HOF's date controller. |
|
To solve the problem of having a custom controller that needs both date and some other functionality, why don't we look at mixing in functionality rather than linear extension, in the interim anyway |
|
Better because it fixes date controller, and there isn't a "direct" inheritance from date to hof base
I like because we remove the linear inheritance. But how do we accomplish this, without engineering ths shit out of it?
We need to be pragmattic. I'm trying hard to understand both arguments. I want the date controller stuff fixed. @joe hof date-controller validators don't work properly, we need this code, but whether that code should be designated at the level we are proposing is my concern. In conclusion, I will accept the proposition, but then I believe we should prioritise a component based architecture. But I don't want that to be an ad-hoc process. We need a discussion around the architecture so we don't end up in a mess pushing pr's based on a single opinion. Let's get concensus first |
|
I didn't think that HOF's date controller was particularly broken. Just that it was an ugly solution? (Mind you I don't think it can handle more than one date per page.) |
|
@daniel-ac-martin it's got bugs though. Leaps being one example. |
|
@easternbloc: Ah okay. It needs to die even more than I thought then. |
|
Well I'm going to revamp this code at some point and either update this gigantic PR or close it and start a new one. In the meantime you guys can have a think about whether you want to persist in the insanity that is the date controller or not. (But the idea of it living there is pretty demotivating to be honest.) And if you do I reserve the right to make fun of both you and HOF to anyone who will listen.
;-) Seriously though there is a chance we can avoid all this stuff anyway. I need to have a play. |
|
And we get to tell people you wanted HOF base controller to inherit from date controller |
|
I wasn't aware it was broken, it works in our use case.
As I mentioned earlier this solution would make the most sense based on templateMixins providing an |
Changes the base controller to inherit from the new hmpo-date-controller
module instead of hmpo-form-wizard's controller.
This provides the base controller with support for dates input via
hmpo-template-mixin's 'input-date' mixin. Thus making the
date-controller redundant.
We should consider removing the date controller in a future release.
Note: As a part of reviewing this PR people should probably review
hmpo-date-controller itself. (Though I'm not sure how best we can do
that. - Suggestions?)