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

RESTWS-688: Enable installation of modules through rest calls #315

Closed
wants to merge 1 commit into from

Conversation

malmike
Copy link
Contributor

@malmike malmike commented Dec 19, 2017

JIRA TICKET NAME:

RESTWS-688: Enable installation of modules through rest calls

SUMMARY:

Currently a module could not be installed through a REST call. When a REST call is made the module is to be downloaded on to the server, loaded and started.

@dkayiwa
Copy link
Member

dkayiwa commented Dec 19, 2017

@malmike have you looked at the above travis failure?

@malmike
Copy link
Contributor Author

malmike commented Dec 19, 2017

@dkayiwa: Am having an issue creating a mock object ModuleUtil which is causing an issue with the test. Need advice on how to implement it. The mock object is made at this point

@dkayiwa
Copy link
Member

dkayiwa commented Dec 19, 2017

Did you see mksd's response on talk?

@malmike
Copy link
Contributor Author

malmike commented Dec 19, 2017

Yes, I have been going through the solutions. Am having difficulty installing powermock into the project, and based on our discussion it might not be good to mix it with context sensitive tests.

@dkayiwa
Copy link
Member

dkayiwa commented Dec 19, 2017

Have you reported that on talk?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 40.446% when pulling cabe819 on malmike:RESTWS-688 into 4f9abc0 on openmrs:master.

Copy link

@aenkya aenkya left a comment

Choose a reason for hiding this comment

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

LGTM

catch (MalformedURLException e) {
// TODO Auto-generated catch block
e.printStackTrace();
throw new IllegalRequestException(e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

@malmike you should use a logger instead of e.printStackTrace() (like here for example).

@mkiterian
Copy link

Looks good 👍

@patlub
Copy link

patlub commented Dec 20, 2017

Looks Fine

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 40.447% when pulling 8590175 on malmike:RESTWS-688 into 4f9abc0 on openmrs:master.


@Resource(name = RestConstants.VERSION_1 + "/moduleinstall", supportedClass = ModuleInstall.class, supportedOpenmrsVersions = {
"1.10.*", "1.11.*", "1.12.*", "1.8.*", "1.9.*", "2.0.*", "2.1.*" })
public class ModuleInstallResource1_8 extends BaseDelegatingResource<ModuleInstall> implements Creatable {
Copy link
Member

@dkayiwa dkayiwa Dec 20, 2017

Choose a reason for hiding this comment

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

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 wanted to use the existing resource but it did not pass the delegate of the install URL and was passing an array for the modules. I can modify the delegates but wouldn't that affect the existing functionality.

Copy link
Member

Choose a reason for hiding this comment

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

I do not seem to understand. Can you explain more and give details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basing on the delegates passed at this line, modules takes an array and I would have to add a delegate for install URI like what I did here. It would also mean that I would have to modify the module action class. Am not sure how the changes would affect the existing functionality and I found it easier to create a new resource. But I will create a new branch that adds the update functionality to the ModuleActionResource1_8 and see how it pans out.

Copy link
Member

Choose a reason for hiding this comment

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

Yes of course you need to modify the switch statement in ModuleActionResource1_8 by including a case for installation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will be working on the changes

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 40.456% when pulling 37ac262 on malmike:RESTWS-688 into 4f9abc0 on openmrs:master.

@dkayiwa
Copy link
Member

dkayiwa commented Jan 3, 2018

Closing because of #316

@dkayiwa dkayiwa closed this Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants