Request for comments - Add Custom Fields in Account JSON response

[Trifon Trifonov]

Dear KillBill developers,

current custom field API requires at least 2 API call to be made in order to retrieve Account data and Custom fields for specific account.

In order to decrease number of API calls I have modified KillBill Account REST API and now JSON includes list of custom fields(customFieldId, name and value).

Example response is below:

{"accountId":"85d6d2e7-5d1d-4200-a8bf-0874bcd6df50"

,"externalKey":"123456.2","accountCBA":0,"accountBalance":0

,"name":"Trifon Trifonov","firstNameLength":6,"email":"m...@example.com"

,"billCycleDayLocal":0,"currency":"BGN","paymentMethodId":null

,"timeZone":"+02:00","address1":"","address2":"","postalCode":"9999"

,"company":"","city":"Sofia","state":"Sofia","country":"Bulgaria","locale":"BG"

,"phone":""

,"isNotifiedForInvoices":false,"isMigrated":false

,"customFields":[

  {"customFieldId":"a09faa85-7ded-4e3a-a169-b6dad5b34bb0","key":"MOL","value":"MOL-Value"}

 ,{"customFieldId":"a09faa85-7ded-4e3a-a169-b6dad5b34bb1","key":"VAT-ID","value":"VAT-ID-Value"}

]

}

I have also added Query parameter (accountWithCF) to the account request:

http://localhost/1.0/kb/accounts?externalKey=123456&accountWithCF=true"

Another idea which I have is to allow update of CustomField. Right now I can't find PUT request for customFields.

I will be happy to know your comments.

Kind regards,

Trifon

[stephane brossier]

Trifon,

While i agree that making two calls to retrieve both account data and custom fields attached to the account is not the most efficient, it would *only* make sense to make such a change if it is propagated to ALL KB objects (for symmetry and completeness) since we can attach custom fields to anything (account, subscription, invoice, payment, ...).

Also, the same issue now arises for tags where tags can also be associated to all KB objects. 

Then, should we now restrict ourselves to the calls that retrieve data (GET) or also allow for instance to create an account with custom fields (in one call instead of 2) ?

More generally, there is a question of how simple do we want to keep the API versus how efficient the calls should be (and what should be grouped within one call). For all the non idempotent calls (POST, PUT), grouping calls would also give rise to a semantics issue in case a partial failure (where only one subset of that state was committed).

So in the end i have some mixed feelings about accepting a PR towards that end. At a bare minimum it would have to be limited to GET (idempotent calls), but include both custom fields and tags, span all the objects, and include the changes in the java and ruby clients (and minimum amount of testing).

Thoughts?

Stéphane

--
You received this message because you are subscribed to the Google Groups "Kill Bill developers mailing-list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to killbilling-d...@googlegroups.com.
To post to this group, send email to killbil...@googlegroups.com.
Visit this group at http://groups.google.com/group/killbilling-dev.
To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-dev/6f6d5ade-b801-4fdd-a022-2fe5d646efed%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Trifon Trifonov]

Hi Stéphane,

> At a bare minimum it would have to be limited to GET (idempotent calls), but include both custom fields and tags, span all the objects, and include the changes in the java and ruby clients (and minimum amount of testing).

This is fine for me. I can develop this changes for all Java classes.

I will need a bit more time for Ruby, but I can try.

Just to note that this fields will be returned only if request includes new query parameter: accountWithCF=true

Default value of new query parameter is false.

Similarly I will add accountWithTags.

Or maybe it is better name of this new query parameters to be withCF/withcustomFields and withTags as this parameters can be used on all KillBill GET requests?

I have question on the implementation style.

Right now my code looks like below:

    public Response getAccountByKey(@QueryParam(QUERY_EXTERNAL_KEY) final String externalKey,

                                    @QueryParam(QUERY_ACCOUNT_WITH_BALANCE) @DefaultValue("false") final Boolean accountWithBalance,

                                    @QueryParam(QUERY_ACCOUNT_WITH_BALANCE_AND_CBA) @DefaultValue("false") final Boolean accountWithBalanceAndCBA,

                                    @QueryParam("withCF") @DefaultValue("false") final Boolean withCustomFields,

                                    @QueryParam("withTags") @DefaultValue("false") final Boolean withTags,

                                    @QueryParam(QUERY_AUDIT) @DefaultValue("NONE") final AuditMode auditMode,

                                    @javax.ws.rs.core.Context final HttpServletRequest request

    ) throws AccountApiException {

        final TenantContext tenantContext = context.createContext(request);

        final Account account = accountUserApi.getAccountByKey(externalKey, tenantContext);

        final AccountAuditLogs accountAuditLogs = auditUserApi.getAccountAuditLogs(account.getId(), auditMode.getLevel(), tenantContext);

        //@Trifon - Return custom fields together with the main Account fields!

        List<CustomFieldMinimalJson> customFieldList = new ArrayList<CustomFieldMinimalJson>();

        if (withCustomFields) {

        final List<CustomField> customFields = customFieldUserApi.getCustomFieldsForAccount(account.getId(), context.createContext(request));

        for (CustomField customField : customFields) {

        customFieldList.add( new CustomFieldMinimalJson(customField.getId(), customField.getFieldName(), customField.getFieldValue()));

        }

        }

Can you please advice is this is acceptable or you would recommend other implementation?

Kind regards,

Trifon

[stephane brossier]

Trifon,

I took a closer look, and i am actually not sure this is a change we are ready to accept. The changes are actually quite extensive: All resources need to be modified, and all models would also need to be enhanced to include different versions where we return custom fields and also potentially tags. The change you suggested below goes in the right direction but is incomplete in the sense that you would also need to modify the AccountJson.

This is not the first time we had requests for 'combo' calls, and while they improve performance they also make the API/code more complex/heavy, so we have always decided to stay away from them.

Stéphane

To view this discussion on the web visit https://groups.google.com/d/msgid/killbilling-dev/c060fa0c-4267-463d-b690-0b659680abbe%40googlegroups.com.

[Trifon Trifonov]

Stéphane

>The change you suggested below goes in the right direction but is incomplete in the sense that you would also need to modify the AccountJson.

Yes, I have also changed AccountJson.

>This is not the first time we had requests for 'combo' calls, and while they improve performance they also make the API/code more complex/heavy, so we have always decided to stay away from them.

Thank you for your response!

I respect your view.

Please let me know if in future you decide to include such change, I will be more than glad to contribute it.

For now I will keep it in my github repository.

Kind regards,

Trifon