OBPIH-4950 Fix searching attributes by name

[kchelstowski]

No description provided.

[jmiranda]

There's a bit to unpack here

1. Do we want a separate service for attributes? Maybe, but needs discussion.

2. Is this the right design for the getAttributes method? Probably not. Pagination parameters as primary arguments to a service method seems incorrect. Creating a new map for pagination parameters with a default value seems like a better approach.

3. Do we want to pass the name as an argument? Or would it be better to pass search terms?

List<Attribute> getAttributes(String searchTerm, Map params = null)

That way we can

String searchTerm = params.q
getAttributes(searchTerm)
getAttributes(searchTerm, [max: 10, offset: 0])

Actually, we might even want to allow the searchTerm to be null by default so can just get all attributes.

getAttributes()

Don't make any of these suggestions. I just wanted to point out that we're refactoring without discussing / designing. I don't want to refactor until we've started to solve some of the underlying problems of our API.

And since we're not going to have much time for tech huddle / tech debt over the next few months, I would recommend we go with the easy route until we can settle some of these conversations.

List<Attribute> attributes = Attribute.findAllByNameIlike("%" + params.q + "%", params)

[kchelstowski]

@jmiranda
2. I was even thinking about a command object for that, that would have those pagination and the name properties - this could grow up easily and we wouldn't have to pass X parameters to the service, but since there were only 3 arguments for now, I didn't go for that. Let me know what you think about that.
3. Attribute.findAllByNameLike("%" + params.q + "%", params) - the problem with that is that if the params.q is null, it would search by name like null, so we'd get an empty list + findAllBy* doesn't return the totalCount which is needed for the pagination.

With that being said, let me know what path I should follow tomorrow.

[jmiranda]

Sorry, I guess if dynamic finders don't return totalCount then we'll need to

List<Attribute> attributes = attributeService.searchAttributes(params.q, [max: params.max, offset: params.offset]) 

And then implement a criteria query

Attribute.createCriteria().list(params) {
    ilike("name", "%" + query + "%")
}

or where query or like you did below.

def query = Attribute.where { 
    ilike("name", "%" + query + "%")
}
return query.list(params)

or like you did

if (params.name) {
    return [Attribute.findAllByNameIlike(...), Attribute.countByNameIlike(...)]
}
return [Attribute.list(params), Attribute.count()]

Or feel free to add an AttributeDataService like this (minus the interface).
https://stackoverflow.com/questions/70778852/pagination-with-grails-gorm-dataservices-query-annotation

If you go that route, the method should look the same as above

@Query("from ${Attribute} attribute where attribute.name like $q")
List searchAttributes(String q, Map params) // or args, if that's required 
```
I guess we'll need to add the %'s when we invoke the method which is probably how we should do it with the other approaches as well.

[kchelstowski]

@jmiranda as I DMed you yesterday - if we wanted to use findAllByNameIlike, we would still have to something like:

if (params.name) {
  return [Attribute.findAllByNameIlike(...), Attribute.countByNameIlike(...)]
}
return [Attribute.list(params), Attribute.count()]

because otherwise we'd search by the attributes with null name.
Let me know if it still looks fine to you, if so, I will revert the service approach and go with the findAllBy + countBy approach.
Additionally - after the discussion from yesterday - let me know if I should create any tech debt ticket to determine the coding conventions for the stuff we discussed, or if we have it somewhere already.

[drodzewicz]

We have been using a ternary operator for assigning default values for the params across the whole application so this isn't a change request but rather a suggestion to use the built-in default value parameter for these cases.
So instead of

params.max = Math.min(params.max ? params.int('max') : 10, 100)
params.offset = params.int('offset') ?: 0

we can do

params.max = Math.min(params.int('max', 10), 100)
params.offset = params.int('offset', 0)

And if we have cases where we need to assign a default value for a string, unfortunately, there is no method params.string('my_string_param', 'default string value')
but rather
params.getOrDefault('my_string_param', 'default string value')

I am making this comment just to point out that this is available and this way makes the code a bit more sophisticated although it is an insignificant change.

[jmiranda]

params.getOrDefault() is ugly af but I'm ok with using it. Odd that there isn't an equivalent for strings. I'll probably continue doing string parameters like JSB recommends in the following response.

https://stackoverflow.com/a/24266854/136597