Issue/3167 filter app datapoints (Connect #3167) #3196
Conversation
…ilter-app-datapoints
| return resp; | ||
| } | ||
|
|
||
| private List<SurveyedLocaleDto> getSurveyedLocaleDtosList(List<SurveyedLocale> slList, | ||
| Long surveyGroupId) { | ||
| protected static List<SurveyedLocaleDto> getSurveyedLocaleDtosList(List<SurveyedLocale> slList, Long surveyId) { |
There was a problem hiding this comment.
Just out of curiosity, why is this method static?
There was a problem hiding this comment.
Ok I understood why, see my comment below why I may suggest to do it differently.
|
|
||
| package org.akvo.flow.api.app; | ||
|
|
||
| import java.util.Date; |
There was a problem hiding this comment.
This seems to be unused
| resp.setCode(String.valueOf(HttpServletResponse.SC_OK)); | ||
| resp.setResultCount(slList.size()); | ||
|
|
||
| List<SurveyedLocaleDto> dtoList = DataPointServlet.getSurveyedLocaleDtosList(slList, surveyId); |
There was a problem hiding this comment.
In my opinion, there should be another class/classes containing methods used by both servlets. This will help have smaller classes and avoid static methods which are harder to test.
There was a problem hiding this comment.
Sure. Just expected the old servlet would be removed soon. Why are these methods harder to test when static? They don't have any side effects.
There was a problem hiding this comment.
When writing tests one usually initialize to class to test with a constructor but of course there are tools like powermock and then static classes methods can also be tested. So there could be a static utility class with utility methods but it seems unusual to me that a servlet calls another one. If you decide to remove the other servlet, then the methods would not need to be static any more. Also what disturbs me a bit is that DataPointServlet is used both with static methods and instantiated via constructor.
There was a problem hiding this comment.
ok to sum up: if they are only utility methods -> we can move them to a static utility class, I guess in this case this would require less rework and makes sense. I still think they are not exactly utility classes since they access the database and read data and in this case if we wanted to mock some responses for them, they should be made into an object with constructor but it would require a change in architecture so maybe not needed right now.
Before the PR (what is the issue or what needed to be done)
No way for app to get only assigned data points.
The solution
Add a new endpoint with modern names.
Moved most of the old endpoint (SurveyedLocaleServlet) methods to the new service (DataPointServlet) and share them.
Screenshots (if appropriate)
Checklist