META-224:Exporting metadata packages on-the-fly should not require yo…#45
META-224:Exporting metadata packages on-the-fly should not require yo…#45reagan-meant wants to merge 1 commit into
Conversation
reagan-meant
force-pushed
the
META-224
branch
from
9675468 to
10f6209
Compare
| } | ||
| } | ||
| } | ||
| 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...
reagan-meant
force-pushed
the
META-224
branch
2 times, most recently
from
295dad4 to
fc0cc85
Compare
mogoodrich
left a comment
mogoodrich
left a comment
There was a problem hiding this comment.
One small comment, but generally looks good me!
Also, do we currently have a "real-world" user for this use case? It would be good to have someone test it will real data.
| } | ||
| } | ||
| } | ||
| 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 |
mogoodrich
left a comment
mogoodrich
left a comment
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 |
mogoodrich
left a comment
mogoodrich
left a comment
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? |
mogoodrich
left a comment
mogoodrich
left a comment
There was a problem hiding this comment.
LGTM, thanks @reagan-meant !
3 participants
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