Skip to content
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

Profile Manager Error Handling #14

Merged
merged 3 commits into from
Oct 25, 2015
Merged

Profile Manager Error Handling #14

merged 3 commits into from
Oct 25, 2015

Conversation

llvasconcellos
Copy link
Contributor

In depth refactoring for better error handling and displaying on the Profile Manager.

Other refactoring was made to improve profiles.jsp readability.
Also was introduced the ability to setup a custom path for the profile storage folder.
The custom folder configuration must be in openmrs-runtime.properties under profile_manager.profile_dir.
This changes is related to issue #12.

…Profile Manager.

Other refactoring was made to improve profiles.jsp readability.
Also was introduced the ability to setup a custom path for the profile storage folder.
The custom folder configuration must be in openmrs-runtime.properties under profile_manager.profile_dir.
This changes is related to issue #12.
@llvasconcellos
Copy link
Contributor Author

This will be difficult to understand diff. I think, to better understand the work is to read the files separately like a before and after compare.

@llvasconcellos
Copy link
Contributor Author

To test the error messages simply do a new dev env setup and see Profile Manager output the problems that it finds regarding things that are missing like the folder, permissions, missing scripts, etc.

@llvasconcellos
Copy link
Contributor Author

To set a custom profile directory now we have the option to set the profile_manager.profile_dir directive in openmrs-runtime.properties:

profile_manager.profile_dir=/usr/share/buendia/profiles

This is not mandatory. Profile Manager will use the default directory if no custom is set.

@zestyping
Copy link
Contributor

Good call on adding the configuration property, Leonardo! I like this solution to the Mac vs. Linux problem.

I've made a few other comments; please have a look.

private File profileDir;

@ModelAttribute
public void getInitializedMyObject() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious how or when this method gets called. How about making this a getter instead, i.e. public File getProfileDir()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Sorry, I forgot to change the name of the method!

import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest you to import the specific classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice that. Again I blame IntelliJ. But I wonder why it did this. I'll look into that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a setting in IntelliJ for this under Preferences > Code Style > Java on the "Imports" tab, named "Class count to use import with '*'".

The settings file at https://github.com/projectbuendia/buendia/blob/dev/intellij-idea-settings.jar should fix this too — it sets the class count to 99 so that import with * is basically never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I fixed it.

@@ -108,7 +136,7 @@ public View post(HttpServletRequest request, HttpServletResponse response, Model
}

/** Chooses a filename based on the given name, with a "-vN" suffix appended for uniqueness. */
String getNextVersionedFilename(String name) {
private String getNextVersionedFilename(String name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private methods should be after public ones.

@viniciusboson
Copy link
Contributor

@llvasconcellos, did you import the project's settings (android-studio-settings.jar for client project and intellij-idea-settings.jar for openmrs project)?
It may avoid those auto indentation issues.

List<FileInfo> files = new ArrayList<>();
for (File file : PROFILE_DIR.listFiles()) {
files.add(new FileInfo(file));
String currentProfile = Context.getAdministrationService().getGlobalProperty(GlobalProperties.CURRENT_PROFILE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed, although you didn't change this either, you could use getCurrentProfile() here if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@zestyping
Copy link
Contributor

Thanks for the quick response, Leonardo!

@viniciusboson
Copy link
Contributor

@llvasconcellos, combining these two PRs #14 and #15, the profile was successfully imported on a mac environment set up from scratch.

@llvasconcellos
Copy link
Contributor Author

@viniciusboson No, I did not import the settings. I'm doing it right now. Thanks for reminding me and thanks for your input on the Mac side of the force!

llvasconcellos added a commit that referenced this pull request Oct 25, 2015
…file-manager-error-handling

Profile Manager Error Handling
@llvasconcellos llvasconcellos merged commit 7400820 into projectbuendia:dev Oct 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants