TRUNK-6304 Refactor code to use Storage Service #4944
Conversation
|
It contains changes from both TRUNK-6300 and TRUNK-6304. They are in separate commits as I'm hoping to backport TRUNK-6300. |
rkorytkowski
changed the title
| File complexObsDir = OpenmrsUtil.getDirectoryInApplicationDataDirectory(adminService | ||
| .getGlobalProperty(OpenmrsConstants.GLOBAL_PROPERTY_COMPLEX_OBS_DIR)); | ||
| File createdFile = createImage(complexObsDir, "openmrs_logo_small.gif"); | ||
|
|
||
| Obs complexObs = obsService.getComplexObs(44, ComplexObsHandler.RAW_VIEW); |
There was a problem hiding this comment.
getCompexObs failed due to missing file with a new implementation. It appears that the old implementation didn't complain that the file didn't exist. I'm confused whether we should fail if the file is missing or silently ignore.
There was a problem hiding this comment.
In order to reduce the possibility of unintended side effects of introducing a new storage service, i would preserve the existing behaviour. Then if we need to change it to fail if the file is missing, i would do that as a separate ticket and pull request. 😊
|
|
||
| private final MimetypesFileTypeMap mimetypes = new MimetypesFileTypeMap(); | ||
|
|
||
| public LocalStorageService(@Value("${storage_dir}") String storageDir, @Autowired StreamDataService streamService) { |
There was a problem hiding this comment.
How can we define the storage_dir? Do we have a property source?
There was a problem hiding this comment.
I don't think we have a property source properly configured in openmrs-core. In such a case the way to define it is to use environment or system variable storage_dir. It's a typical Docker approach of configuring an app.
We probably want to configure property source from openmrs-runtime.properties, but that deserves a separate issue.
There was a problem hiding this comment.
What if we use @Value("${storage_dir}") after implementing that?
There was a problem hiding this comment.
It would first use value from the env property if not found try sys property and finally openmrs-runtime.properties. See also https://docs.spring.io/spring-boot/docs/2.6.1/reference/html/features.html#features.external-config.typesafe-configuration-properties.relaxed-binding
| @@ -36,6 +43,12 @@ public class AbstractHandler { | |||
|
|
|||
| protected NumberFormat nf; | |||
|
|
|||
| @Autowired | |||
There was a problem hiding this comment.
Is there a specific reason for keeping these access modifiers as default?
There was a problem hiding this comment.
Default lets us easily mock them in tests that we put in the same package. I included them in a constructor so it's not needed in this case, but sometimes constructor would be too long. I don't know if we have a convention around that, but it's something I find quite common in spring code. They could be protected as well, but then mocking is only feasible via the constructor.
rkorytkowski
force-pushed
the
TRUNK-6304
branch
from
f2c308b to
5a93c36
Compare
|
It's no longer a draft. There's one final question I have around failing if file is missing. See above. Other than that I consider it ready. #4920 should be merged first. |
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-
Checklist: I completed these to help reviewers :)
[ x] My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amend[ x] I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amend[x ] I ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
[x ] All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
[x ] My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master