-
Notifications
You must be signed in to change notification settings - Fork 186
add way to refer to custom fields by their names #607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
d71f21b to
048ef78
Compare
bfeeb82 to
d471f70
Compare
4eb2647 to
85cd4bc
Compare
85cd4bc to
68805ae
Compare
Yes. |
| def find_by_name(field_name) | ||
| @ticket.refresh_custom_fields_metadata unless @ticket.account_data.has_key?(:custom_fields) | ||
|
|
||
| custom_field_id = @ticket.account_data[:custom_fields][field_name] | ||
| custom_field = @ticket.custom_fields.find { |cf| cf["id"] == custom_field_id } | ||
| raise ArgumentError if custom_field.nil? | ||
|
|
||
| custom_field | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could check if custom_field_id is nil before doing find to save us some work if we're never going to find anything. We can add a message to ArgumentError to help with debugging.
|
|
||
| it "stores custom fields metadata on a client" do | ||
| _custom_field_value = @ticket.custom_field["Custom field name"] | ||
| expect(@ticket.account_data[:custom_fields]["Custom field name"]).to eq(9961714922394) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does 9961714922394 work? 😅
Thomascountz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some questions. I'd love to benchmark things once we understand how folks are using client objects.
Usage
How it works
When
#custom_fieldis called for the first time on any ticket for the client instance, we fetch allticket_fieldsand store a mapping from field names to their IDs on that client instance. Following#custom_fieldwill reuse this mapping.You can call
client.refresh_custom_fields_metadataif you expect that definition of fields have changed since the data was loaded.To enable bracket syntax,
custom_fieldreturns aCustomFieldAccessorobject with[](field_name)and[]=(field_name, value)methods.The mapping is stored inside
account_datahash on a client: I’ve decided to give a more generic name to give future additions an obvious placement. However, we expose a custom fields-specificrefresh_custom_fields_metadatamethod on aResourceso that resources can refresh only that part ofaccount_datawhich is relevant for them.Should we preload this metadata?
Initially I added a config key that would enable this preloading and fetch ticket fields during client initialization. We can potentially go back to this, but I think that memory usage is low enough not to warrant an additional configuration flag.
Note on memory usage
A custom fields mapping stored on a client looks like this:
(It includes all fields, not only custom ones).
If you have 1000 custom fields with 30-character names, this mapping will need about 40 KB. This shouldn’t matter for most cases, but if you simultaneously have in memory thousands of clients for accounts with thousands of custom fields, memory usage of these mappings might become noticeable.
Alternative implementation: no accessor, worse method names
I’ll squash these commits before merging but for now I wanted to have a commit with a simpler approach (but arguably worse API).
This doesn’t require an additional accessor class. I think
ticket.custom_field["field name"]way is much better and worth a bit of complexity.