feat: VOL-5664 welcome page now appears for external users to agree terms and conditions #419
Conversation
…erms and conditions
|
|
||
| public function agreeTermsAndConditions(): void | ||
| { | ||
| $this->termsAgreed = true; |
There was a problem hiding this comment.
As with other mutable operations on this class, I feel we should return $this;. Means we can retain/have a fluent interface.
There was a problem hiding this comment.
makes sense, I'll change it
| @@ -32,6 +32,10 @@ public function indexAction() | |||
| throw new \Exception('Unable to retrieve identity'); | |||
| } | |||
|
|
|||
| if (!$identity->getUserData()['termsAgreed']) { | |||
There was a problem hiding this comment.
Should this be done by a listener/middleware instead (or in addition to)? This can easliy be circumvented. Meaning users could potentially be using the service without explicitly agreeing to the terms of service. At least with the middleware approach, we intercept all requests from a new user until they have explicitly agreed to the terms.
| * | ||
| * @ORM\Column(type="boolean", name="terms_agreed", nullable=false, options={"default": 0}) | ||
| */ | ||
| protected $termsAgreed = 0; |
There was a problem hiding this comment.
Not a blocker at this stage, but there was a conversation with Kab a while back, and an idea was to have this is a DateTime/Timestamp. Then opens possibbility in future for the business to update the terms of service (say via some content mangement on internal), without having to set all the users back to false.
There was a problem hiding this comment.
I don't know if I was in on that, if so I can't remember. But either way, what you suggest makes a lot of sense, and would be better all round. It'd require a few changes of course, including to ETL patch, so I'll check with Kab and see how bothered he his about it.
| $consultantUser = $this->getRepo()->fetchById($this->result->getId('user')); | ||
| $operatorTcRole = $this->getRepo('Role')->fetchByRole(RoleEntity::ROLE_OPERATOR_TC); | ||
| $consultantUser->setRoles(new ArrayCollection([$operatorTcRole])); | ||
|
|
||
| //the consultant has already accepted terms and conditions | ||
| $consultantUser->agreeTermsAndConditions(); |
There was a problem hiding this comment.
We need something similar for Operator Administrator (standard/existing create user command).. If they sign up for their own account, they too will have already ticked the terms of service checkbox.
There was a problem hiding this comment.
I see what you mean - I'll have a look
Description
Welcome page now appears for external users to agree terms and conditions.
In terms of the login controller I've done enough to get the tests working, but the tests for that as a whole are awful and don't really test anything properly. We need to replace them rather than add to them.
Related issue: VOL-5664