TRUNK-5382 add extra utility method to exclude retired concepts #2654
Conversation
|
Would be good if the commit message referenced the Jira. |
|
|
||
| List<Concept> setMembers = c.getSetMembers(true); | ||
|
|
||
| Assert.assertEquals(4, setMembers.size()); |
There was a problem hiding this comment.
Can you confirm that those four Concepts are a mix of retired and unretired Objects?
You should prove that this setMembers contain the unretired concepts.
There was a problem hiding this comment.
Assert.assertEquals(2, c.getSetMembers(false).size());
this will exclude the retired concepts
There was a problem hiding this comment.
Here, what I am saying is you are trying to verify your implementation. So you should prove this before using another method.
i.e : let's count the retired and unretired concepts from setMembers and prove, it contains both.
There was a problem hiding this comment.
Assert.assertFalse(setMembers.get(0).getRetired()); // not retired
Assert.assertFalse(setMembers.get(1).getRetired()); // not retired
Assert.assertTrue(setMembers.get(2).getRetired()); // retired
Assert.assertTrue(setMembers.get(3).getRetired()); // retired
|
|
||
| List<Concept> setMembers = c.getSetMembers(false); | ||
|
|
||
| Assert.assertEquals(2, setMembers.size()); |
There was a problem hiding this comment.
How can you confirm that these two objects are unretired objects and no more retired objects in this list?
There was a problem hiding this comment.
setMembers.forEach(member -> Assert.assertEquals(true, !member.getRetired()));
this will go through each member and check if they are not retired
There was a problem hiding this comment.
I have made the changes requested, let me know if there are other changes
saidrobley
changed the title
saidrobley
changed the title
saidrobley
force-pushed
the
TRUNK-5382
branch
2 times, most recently
from
9b6646b
to
c828950
Compare
|
Have you thought of coveralls failure? |
|
I have fixed that failure |
| @@ -1556,6 +1556,25 @@ public void setId(Integer id) { | |||
| } | |||
| return Collections.unmodifiableList(conceptMembers); | |||
| } | |||
|
|
|||
| /** | |||
| * If <code>includeRetired</code> is true, then the returned object is the actual stored list of | |||
There was a problem hiding this comment.
Can please clean up the javadocs? The second phrase is probably unnecessary, the javadocs need to mention what the method does, i.e. returns set members and includes retired ones if includeRetired is set to true.
There was a problem hiding this comment.
@saidrobley did you see the above comment?
| /** | ||
| * When asked for a collection of compatible names, the returned collection should not include | ||
| * any incompatible names. | ||
| * |
There was a problem hiding this comment.
Can you please revert these auto format changes? Did you format your code?
There was a problem hiding this comment.
It does appear that there are spaces there in master
https://github.com/openmrs/openmrs-core/blob/master/api/src/test/java/org/openmrs/ConceptTest.java#L40
saidrobley
force-pushed
the
TRUNK-5382
branch
2 times, most recently
from
361bd3b
to
399cd5f
Compare
remove comments revert auto format changes revert auto format changes
|
@saidrobley did you see @wluyima's comment? |
|
@saidrobley are you still working on this? |
|
@dkayiwa yes, I have made all the changes requested |
| * @return List<Concept> the Concepts that are members of this Concept's set | ||
| * @should return set members and includes retired ones if includeRetired is set to true | ||
| * @should not return retired members if includeRetired is false | ||
| */ |
There was a problem hiding this comment.
Can you add a since annotation to indicate the openmrs version when this method is introduced?
There was a problem hiding this comment.
hi, it's included at line 1563
|
@saidrobley did you see the above comments? |
|
@dkayiwa since annotation to indicate the openmrs version is at line 1563 |
|
Closing because the author is no longer working on it. |
TRUNK-5382 Extra utility method for excluding retired concepts while fetching setmembers
Description of what I changed
Created utility method called getSetMembers(boolean includeRetired). If includeRetired is true it returns the same as getSetMembers(), if false it should return excluding retired members.
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-5382
Checklist: I completed these to help reviewers :)
[x ] My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).
[x ] My IDE is configured to follow the code style of this project.
[ 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)
[x ] I ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
[x ] All new and existing tests passed.
[ x] My pull request is based on the latest changes of the master branch.