PP-13313: Worldpay details index page #4365
Conversation
oswaldquek
force-pushed
the
PP-13313-link-worldpay-account-not-yet-started
branch
2 times, most recently
from
d71d578 to
9a3bb58
Compare
|
|
||
| const { getActiveCredential } = require('@utils/credentials') | ||
|
|
||
| class WorldpayTasks { |
There was a problem hiding this comment.
This probably doesn't need to be a class, if it just produces a list of tasks from a gateway account object it can just be a single function
There was a problem hiding this comment.
It probably doesn't need to be a class, but I don't think it's wrong either? My thinking was to encapsulate all the logic in a class to keep the controller simple. Sure a list of tasks can be returned but I'd like to keep the controller simple by doing worldpayTasks.incompleteTasks rather than calculate that in the controller or worse still the nunjucks file.
There was a problem hiding this comment.
you could follow the pattern established for stripe tasks and create a utility function instead of a class: https://github.com/alphagov/pay-selfservice/blob/master/app/utils/simplified-account/settings/stripe-details/tasks.js
There was a problem hiding this comment.
The Stripe tasks are more complex and their state is stored in connector db, so I imagine that's why they'd be handled differently. For me the question to ask when reviewing is the code readable? I think what I've got here currently is easily understandable. I'm not sure it could be classed as bad code either. We can refactor it to be more like the Stripe tasks pattern if we need be (for example when we come to add more Worldpay tasks). Trying to make this more like Stripe tasks is going to hurt my head at the moment.
oswaldquek
force-pushed
the
PP-13313-link-worldpay-account-not-yet-started
branch
from
e49b0ad to
e1c1ff1
Compare
| external_id: SERVICE_ID | ||
| })) | ||
| .withAccountType(ACCOUNT_TYPE) | ||
| .withAccount(new GatewayAccount({ |
There was a problem hiding this comment.
this is a good idea, we should probably update the ControllerTestBuilder to create a new GatewayAccount when calling withAccount instead of relying on individual tests
There was a problem hiding this comment.
I agree, but think this should be done in another PR. If withAccount creates a new GatewayAccount, what happens to the withAccountType method, and should it even exist at all? It touches a lot of test classes. Hence I think it'll be better to fix this in a separate PR (which I'm happy to do).
oswaldquek
force-pushed
the
PP-13313-link-worldpay-account-not-yet-started
branch
from
ae712d1 to
642200e
Compare
app/models/GatewayAccount.class.js
Outdated
| * @property {string} analyticsId - Google analyticsId of the gateway account | ||
| * @property {boolean} toggle3ds - whether 3DS is enabled or not on this gateway account | ||
| * @property {[GatewayAccountCredential]} gatewayAccountCredentials - available credentials for gateway account | ||
| * @property {GatewayAccountCredential} activeCredential - the active credential for the gateway account |
There was a problem hiding this comment.
this is an optional property so should be wrapped in square brackets, see gatewayAccountCredentials above
There was a problem hiding this comment.
Are you sure? I thought [GatewayAccountCredential] means a list of credentials, which a gateway account does have.
There was a problem hiding this comment.
sorry I meant @property {GatewayAccountCredential} [activeCredential]. My B! It's the property name that should be in brackets if optional, not the type
| You need to link your Worldpay account to GOV.UK Pay. | ||
| </p> | ||
|
|
||
| <ul class="govuk-task-list"> |
There was a problem hiding this comment.
we should be using the task list component now that DSv5 is live.
See this PR
This commit deals with a moto-enabled gateway account that hasn't configured the one_off_customer_initiated credentials yet.
oswaldquek
force-pushed
the
PP-13313-link-worldpay-account-not-yet-started
branch
from
642200e to
3541d73
Compare
app/views/simplified-account/settings/worldpay-details/index.njk
Outdated
Show resolved
Hide resolved
This commit deals with a moto-enabled gateway account that hasn't configured the one_off_customer_initiated credentials yet. We just want to show that there's an outstanding task ('Link your Worldpay account with GOV.UK Pay') here; the link currently doesn't go anywhere.