Alfresco / Aikau

Aikau UI framework
GNU Lesser General Public License v3.0
82 stars 62 forks source link

AlfList adds redundant pubSubScope prefix to alfResponseTopic for data load #955

Open AFaust opened 8 years ago

AFaust commented 8 years ago

When an AlfList instance is used with a non-empty pubSubScope, the payload of the data load pub will contain both the alfResponseScope set to the pubSubScope and the alfResponseTopic prefixed with the pubSubScope. Any custom service that uses both alfResponseTopic and alfResponseScope to pub a response with the actual data will not reach the AlfList instance due to the effective response publish topic being double-prefixed with the pubSubScope of the AlfList instance.

The full setup to reproduce this is defined in this gist: https://gist.github.com/AFaust/b2b55bfa15eecbd6c7ef

In short, the relevant config / code snippets are 1) service operation

onLoadSimpleList : function test_SimpleListService__onLoadSimpleList(payload)
        {
            // this response only works for AlfList with an empty pubSubScope
            this.alfPublish(payload.alfResponseTopic + '_SUCCESS', {
                // this is accessed as fallback - only after payload.response.items failed
                items : [/* ... actual items ... */],
                // this is not actually mapped by AlfList - only payload.response.metadata is accessed
                metadata : {}
            }, false, false, payload.alfResponseScope || '');
        }

2) list config

model.jsonModel = {
    widgets : [{
    name : 'alfresco/lists/AlfList',
    config : {
        loadDataPublishTopic : 'LOAD_SIMPLE_LIST',
        // this causes AlfList not to receive the response from the service
        pubSubScope : 'Test',
        widgets : [/* ... list views ... */]
         }
    }]
};

This is using Aikau 1.0.60 on Alfresco 5.0.3

AFaust commented 8 years ago

As a minor aside: I don't know the reason why it doesn't, but the access to the metadata fragment of the payload should also follow the same fallback pattern as the access to items array.

draperd commented 8 years ago

Our primary responsibility is for backwards compatibility, and whilst I completely agree that we've made a bit of a mess of pub/sub scoping as we were finding our way with it, we need to ensure that we don't regress any existing functionality in Share and we also want to be sure that we are not going to break any 3rd party usage of the list that we are not aware of.

Having said all that, I will check out the branch for your pull request, run all the unit tests and try it out against Share to see if I can see if it causes any major problems. If it turns out that we can contain any fallout within existing Aikau widgets then we may choose to take a calculated risk to make the change.

It may be that there is no risk involved, but we need to be 100% sure before going ahead. Until we take the time to do a proper assessment we won't know.

At some stage soon I'd like to start looking to move towards a 1.1 release where we can make potentially more breaking implementations. This might be something that we continue to tolerate until then - but as I said, until we've taken the time to do the full analysis we won't know.

draperd commented 8 years ago

I've put some additional comments on #956 - the PR cannot be merged as is due to the introduction of wide spread test failures. However, it does look like with further updates to DocumentService and other some of the mock services used for testing we would be able to safely make this change.