Hi @manav2modi,

1. I have no significant comments about the model.

1.1. I am not a fan of the json_annotation package but I see that you are in Rokmetro. Let it be so.

1.2. My feeling is that latitude and longitude should not be nullable. Is there a case where we could have a place without a geographical location defined?

1.3. What is the idea of a UserPlace object? Just to indicate whether the current user has visited the place? Why did you decide to add such functionality? Is there any specification on this? If yes, could you place apply it in #475 definition. If not, why is this a hurry to add it while it is not clear if we need it, what exactly, etc.

1.4. Also, I wanted to point out that we would probably need to define a relation between a place and the existing objects that also represent location like Gateway Buildings (wayfinding/buildings API in Gateway BB), for example. Maybe some field like external_id, entetity_id would handle this case, as well as the other that could arise. I consider that this could be handled later when we face up the necessity for this.

2. Regarding the service:

2.1. If ids, types and tags parameters of getAllPlaces API are supposed to contain a number of single objects of that type (like comma separated list?), they would better be of Set<String> type and let the API build the URL parameters properly.

2.2. You should ensure that all methods in Places service do not fire exceptions outside its APIs. Particularly Uri.parse and Place.fromJson are candidates to fire an expectation (and this is one of the reasons I am not a fan of json_annotation). So please make sure you handle all exceptions that potentially could raise.

2.3. I would recommend adding List JSON serialization methods to your Place model (JSON <=> List), instead of applying the current implementation every time you need to deserialize List from JSON. In addition, if a given Place instance does not follow your types expectations (for example if appId or orgId comes as an integer value instead of String) you should omit this Place instance from the result list and let it collect the other valid Places (unlike your implementation does now).

3. In general, I think things are happening in the wrong order (in Bulgaria we say to put the cart before the horse). We should first get some spec of what is expected as a functionality in the client application, then we should design the model and the APIs in Places BB, and just after them to do what you are trying to do now. Have the first two steps been taken yet? If yes, can I get more details, specs, etc. about them?

Hi @manav2modi,

Thanks for the quick update!

2.1. I have made that change.
Is there any particular reason you made these parameters List<String> instead of Set<String> as I suggested? Is the order of these values significant for GET /places API?

Something else that I missed yesterday is the PlacesService name. Could you please remove the Service suffix from this name so that it matches the services naming convention that is currently adopted in the app?