Default account configuration and account status support #959
Conversation
| import org.prebid.server.settings.model.AccountStatus; | ||
| import org.springframework.validation.annotation.Validated; | ||
|
|
||
| @Validated |
There was a problem hiding this comment.
Why this annotation required ?
There was a problem hiding this comment.
Good question. It's been copied along with other annotations that we put on Spring configuration properties POJOs but apparently there are no constraints imposed on any fields in this class so it's redundant. We may put sensible constraints in future and easily overlook adding @Validated annotation though so I'm hesitant about this one.
|
|
||
| private Boolean enforceCcpa; | ||
|
|
||
| private String gdpr; |
There was a problem hiding this comment.
Why cant we use AccountGdprConfig class
There was a problem hiding this comment.
Because we don't want to add @Data @NoArgsConstructor annotations to AccountGdprConfig, i.e. make it a Spring configuration properties POJO.
There was a problem hiding this comment.
Probably you mean AccountAnalyticsConfig because AccountGdprConfig is already have this annotation.
There was a problem hiding this comment.
Good point. AccountGdprConfig shouldn't have had these annotations in the first place.
There was a problem hiding this comment.
Probably my mistake. I copy from GdprConfig which we get from yaml.
But why we don't want to make this @DaTa ? Because of mutability ?
There was a problem hiding this comment.
Yes, it's a convention for value objects throughout PBS codebase to aspire for immutability unless it is impractical.
Another reason to take into consideration - using AccountGdprConfig to represent data in two different places (PBS configuration and account configuration in DB/file/etc) strongly couples these sources and forces changes in one place (account config for example) to affect another place (PBS config) and vice versa. Actually This is what Single Responsibility Principle is called to protect from.
|
|
||
| public Account merge(Account another) { | ||
| return Account.builder() | ||
| .id(ObjectUtils.firstNonNull(id, another.id)) |
There was a problem hiding this comment.
Minor. i'd rather suggest to use ObjectUtils.defaultIfNull(..) instead since we have only 2 possible values.
* Add support for default account configuration values in application config * Take into consideration account status * Update documentation * Address review comments
* Add support for default account configuration values in application config * Take into consideration account status * Update documentation * Address review comments
* Add support for default account configuration values in application config * Take into consideration account status * Update documentation * Address review comments
No description provided.