Standalone Console API & user feedback improvements#9847
Conversation
9313838 to
446b85a
Compare
evandongen
left a comment
There was a problem hiding this comment.
Mooi werk! Paar dingen toegevoegd, daar kunnen we evt samen nog even naar kijken als je wilt.
|
|
||
| @Scheduled(fixedDelayString = "${console.socket.poller.warnings}", timeUnit = TimeUnit.SECONDS, initialDelayString = "${console.socket.poller.startDelay}") | ||
| public void serverWarnings() { | ||
| if (hasNoAvailableWorker()) return; |
There was a problem hiding this comment.
Ik vind dit (zeker zonder accolades) niet echt lekker lezen. Kan je het omdraaien?
Bv:
if (hasAvailableWorker()) {
// doe dingen
}
| @Override | ||
| public final void afterPropertiesSet() { | ||
| environment = applicationContext.getEnvironment(); | ||
| clusterMembers = getGateway().getMembers().stream() |
There was a problem hiding this comment.
Is de lijst van clusterMembers altijd vast of wil je dat dynamisch bepalen?
There was a problem hiding this comment.
De lijst past zich aan zodra een nieuwe member zich aan- of afmeld via hazelcast, het is dus een momentopname
| private List<SecurityRolesDTO> getSecurityRoles() { | ||
| return BusMessageUtils.getAuthorities() | ||
| .stream() | ||
| .map(authority -> new SecurityRolesDTO(authority.getAuthority().substring(5))) |
There was a problem hiding this comment.
| .map(authority -> new SecurityRolesDTO(authority.getAuthority().substring(5))) | |
| .map(SecurityRolesDTO::new) |
| public SecurityRolesDTO(String role) { | ||
| this.name = role; | ||
| this.allowed = BusMessageUtils.hasRole(role); | ||
| } |
There was a problem hiding this comment.
En dan hier:
| public SecurityRolesDTO(String role) { | |
| this.name = role; | |
| this.allowed = BusMessageUtils.hasRole(role); | |
| } | |
| public SecurityRolesDTO(GrantedAuthority authority) { | |
| this.name = authority.getAuthority().substring(5); | |
| this.allowed = BusMessageUtils.hasRole(this.name); | |
| } |
There was a problem hiding this comment.
Oooh ja, dit ziet er natuurlijk een stuk netter uit :D
| Map<String, Object> emptyMap = Map.of(); | ||
| List<Object> emptyList = List.of(); | ||
| returnMap.put("securityRoles", getSecurityRoles()); | ||
| returnMap.put("jmsRealms", emptyMap); |
There was a problem hiding this comment.
Ik zou hier gewoon iedere keer Map.of() meegeven. Dat is sowieso altijd al een referentie naar hetzelfde object: java.util.ImmutableCollections#EMPTY_MAP
zelfde voor list.
There was a problem hiding this comment.
Dat wist ik niet, ik had verwacht dat elke .of() een nieuw object in memory zou stoppen
| this.expiringCertificates = data.expiringCertificates; | ||
| }, | ||
| error: (error: HttpErrorResponse) => { | ||
| if (error.status === 503) { |
There was a problem hiding this comment.
Heb je hier nou echt de hele map nodig uit org.frankframework.console.controllers.SecurityItems#getUnavailableSecurityItems?
There was a problem hiding this comment.
Dit zou inderdaad wat mooier kunnen als we er vanuit gaan dat alles optioneel kan zijn.
Het is op dit moment ook nog onduidelijk of we bepaalde properties wel via dit endpoint willen laten lopen en niet afsplitsen en een nieuwe pagina op de frontend maken.
Omdat dit nog op tafel ligt dacht ik dat het beste was om niet al te veel dit om te gooien
|
|
||
| } | ||
|
|
||
| private ResponseErrorHandler ignoreErrorHandler() { |
There was a problem hiding this comment.
Kan je toelichten waarom je dit gebruikt in een comment?
| return frankApiService.callSyncGateway(RequestMessageBuilder.create(BusTopic.HEALTH)); | ||
| } | ||
|
|
||
| private Map<String, Object> getUnavailableServerInformation() { |
There was a problem hiding this comment.
Hoe langer ik er naar kijk, hoe meer ik me afvraag of dit wel de juiste oplossing is. Waarom doe je dit hier (en in SecurityItems)? Heeft de frontend echt al deze lege attributen nodig?
There was a problem hiding this comment.
De frontend verwacht dat het hele object er niet of wel is, in een normale scenario kan je daar van uitgaan aangezien het verplichte informatie is.
Maar omdat er nu geen ff worker instantie draait heeft het dus placeholders nodig hier of veel meer checks in de frontend voor optionele properties
There was a problem hiding this comment.
Duidelijk. Dan is het nu maar even wat het is denk ik.
|
No description provided.