-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove 'production entities enabled' feature toggle from the Service entity #628
Conversation
2e778b3
to
4472496
Compare
From the Service entity. This setting is never used. All services have the production enitties enabled by default. So this setting is never used, and can be removed
4472496
to
d24e12c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestions / questions, no blockers to merge IMHO
@@ -51,7 +50,7 @@ public function __construct( | |||
private ?string $intakeStatus, | |||
private ?string $contractSigned, | |||
private ?string $surfconextRepresentativeApproved, | |||
private ?string $privacyQuestionsAnswered, | |||
private bool $privacyQuestionsAnswered, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not nullable anymore?
BTW, I heard a statement, that bool variables "sh|c"ould be named with is
in front, like $isPrivacyQuestionAnswered.
Butt hat is probably not for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If is is not nullebale, maybe initilize it with false to be sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I looked it up, and it should always be set. My own testing and the CI functional tests did confirm my assumption. I feel confident to leave it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review @parijke
The 'production entities enabled' feature toggle is not used by the PO's of SPD. They always allow the services to create production entites. Having this featue is illogical, and it can be cleaned up.
Note that a Doctrine migration was created to remove the column from the
service
table.https://www.pivotaltracker.com/story/show/187053888