O3-5723: Improvements in printing feature#28
Conversation
|
@wikumChamith can you take a look? Next part of improvements after feedback gathered from end user. |
| private FopFactory buildFopFactory() throws Exception { | ||
| try (InputStream fopConfigStream = Helper.getInputStreamByResource(FOP_CONFIG_PATH)) { | ||
| if (fopConfigStream != null) { | ||
| URI fontBaseUri = OpenmrsClassLoader.getInstance().getResource(FONT_BASE_PATH).toURI(); |
There was a problem hiding this comment.
Don't we need an NPE guard here?
There was a problem hiding this comment.
right, corrected
| matched.add(obs); | ||
| } | ||
| } | ||
| return matched; |
There was a problem hiding this comment.
Don't we need to return conceptObs here if matched is empty? Otherwise, there could be data loss.
There was a problem hiding this comment.
You're right that data loss is a risk, but returning conceptObs unconditionally would re-introduce the exact bug this method fixes: when a concept is reused across several questions, an empty matched for one question means the data belongs to a sibling question, and returning all observations would print every section's value under each occurrence again. I scoped the fallback: on an empty match we now return conceptObs only when the concept is used by a single question, when the concept is shared by multiple questions, an empty result is expected and we return empty
| Configuration cfg = new DefaultConfigurationBuilder().build(fopConfigStream); | ||
| return new FopFactoryBuilder(fontBaseUri).setConfiguration(cfg).build(); | ||
| } | ||
| log.warn("FOP configuration '{}' not found on classpath; extended glyphs may not render correctly.", FOP_CONFIG_PATH); |
There was a problem hiding this comment.
This is only going to display a server-side log.warn that no clinician sees. Should this fail loudly rather than emit a silently corrupt PDF?
There was a problem hiding this comment.
right, corrected
|
@wikumChamith PR updated |
|
@druchniewicz what do you think about the three new Sonar issues? |
|
@wikumChamith indeed right, I've fixed them. This new 1 issue is not related to changes from this PR. I could fix it by extracting all these little pieces of code into separate methods but honestly I don't think it's more readable |
2 participants
What have been resolved?
Forms that reuse the same concept across several sections previously printed the concatenation of every observation for that concept in each occurrence instead of single value.
Characters outside the base Latin set were printed in wrong way e.g.
>=18was printed as#18or arabic texts likeالنفسية الصحwere printed as##########