META-224:Exporting metadata packages on-the-fly should not require yo… #45
Conversation
reagan-meant
force-pushed
the
META-224
branch
from
9675468 to
10f6209
Compare
| @@ -226,7 +227,18 @@ public void getNewPackage(@RequestParam(required = false) | |||
| } | |||
| } | |||
| } | |||
| } | |||
| if(uuids==null && ids==null && modifiedSince!=null) { | |||
There was a problem hiding this comment.
@reagan-meant , wont you also need to check whether type is not null ??
There was a problem hiding this comment.
@mozzy11 type has to always be specified...it isn't optional
There was a problem hiding this comment.
I think we actually can allow modifiedSince to be null... in which case it would fetch all objects of a certain type.
| } | ||
| if(uuids==null && ids==null && modifiedSince!=null) { | ||
| List<Object> items=Handler.getItems(type, true, null, null, null); | ||
| if (items != null) { |
There was a problem hiding this comment.
did you have to hard code the boolean includeRetired to true ? . i would think we could have it as a variable that has a default behaviour may be false
but can also optionally be configurable via the URL ie
http://localhost:8080/openmrs/ws/rest/metadatasharing/package/new.form?type=org.openmrs.Concept&modifiedSince=01/01/2012&IncludeRetired=True
There was a problem hiding this comment.
@mozzy11 That makes sense...I will change to that...
There was a problem hiding this comment.
@mozzy11 How is this now?
reagan-meant
force-pushed
the
META-224
branch
2 times, most recently
from
295dad4 to
fc0cc85
Compare
| @@ -226,7 +227,18 @@ public void getNewPackage(@RequestParam(required = false) | |||
| } | |||
| } | |||
| } | |||
| } | |||
| if(uuids==null && ids==null && modifiedSince!=null) { | |||
There was a problem hiding this comment.
I think we actually can allow modifiedSince to be null... in which case it would fetch all objects of a certain type.
reagan-meant
force-pushed
the
META-224
branch
from
fc0cc85 to
fd19fe0
Compare
|
@mogoodrich I have included your new suggestion of modifiedSince being null and given a detailed description in the issue's comments |
There was a problem hiding this comment.
Looks good, thanks @mozzy11 !
I can go ahead and merge if there are no objections?
|
sure look fine. no objections |
|
I am delighted to hear that and thanks for the guidance too @mogoodrich @mozzy11 |
There was a problem hiding this comment.
Sorry, took me a while to get to this... I made one suggestion for an optimization, but otherwise looks good to me! Thanks @reagan-meant !
| List<Object> items=Handler.getItems(type, includeRetired, null, null, null); | ||
| if (items != null) { | ||
| for(Object item: items ) { | ||
| if (OpenmrsUtil.compareWithNullAsEarliest(Handler.getDateChanged(item), |
There was a problem hiding this comment.
Can we modify this test to be if "modifiedSince == null || OpenmrsUtil.compareWithNullAsEarliest...". Then we should be able to get rid of the second block (lines 241 thru 248) entirely?
There was a problem hiding this comment.
@mogoodrich I will make the changes shortly...
…u to specify ids/uuids of existing items
|
@mogoodrich Does this suffice? |
META-224:Exporting metadata packages on-the-fly should not require you to specify ids/uuids of existing items. Left legacy compatibility when they are specified
https://issues.openmrs.org/browse/META-224