Skip to content
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

Fix for #955 - removal of redundant pubSubScope prefix in alfResponseTopic #956

Closed
wants to merge 6 commits into from

Conversation

AFaust
Copy link
Contributor

@AFaust AFaust commented Mar 25, 2016

This trivial change fixes #955 by ensuring alfResponseTopic remains clear of any scope prefix, relying on services to handle alfResponseScope properly to target the response publish.
Both #955 and this PR address a very simple item from my "list of Aikau issues" that make developing custom services a bit of a pain when list-oriented data visualisation is commonly used.
Without this change a developer would basically have to check and publish each response twice if the alfResponseScope is non-empty and the pubSub-interaction could have been initiated by an AlfList instance.

@AFaust
Copy link
Contributor Author

AFaust commented Mar 25, 2016

@draperd In my Google spreadsheet you stated that "We've used lists with configured pubSubScope values and have had them work." - I don't know which services have been used, but I guess this simple change might break those cases if those still use inconsistent approaches to handling alfResponseTopic and alfResponseScope

@draperd
Copy link

draperd commented Mar 29, 2016

I've commented on #955, but in summary - our priority is to backwards compatibility - I'll check out the branch and run all the unit tests and try it out against Share (I'll also get the RM team to test it out). I want to make sure that this isn't going to cause any obvious problems - it's not that I don't want to accept the pull request (I definitely do want to start accepting contributions!) but I just want to be sure that this isn't going to have any further issues. This won't get done before the release of 1.0.61, but hopefully we'll get a chance to do this in the next sprint

@draperd
Copy link

draperd commented Mar 29, 2016

I've checked out the code and tested this out against Share and run all the unit tests. On the positive side, the behaviour in Share is good - however, that is partly because of the still limited use of Aikau within Share - and because both the AlfSearchList and the AlfSitesList have their own version of the loadData function. However, there are a lot of unit test failures.

The reason for the failures is because we do have projects that are not directly part of Share that are using the Document Library and the resulting issue appears to be in the way that the DocumentService handles the the scope data that it receives.

It is not passing on the alfResponseScope, just the alfTopic - this means that all instances of our scoped Document Library implementations will break. However, I do think that with the associated update to DocumentService we might be able to get accept a change such as this.

On top of this, it looks like further updates would be required to some of the mock services used for testing (such as aikau/src/test/resources/testApp/js/aikau/testing/mockservices/PaginationService.js) because that is also written with an expectation of the current implementation of AlfList.

So in principle I think that making this change would be acceptable - however, due diligence in running and updating the associated tests is also required.

There are a couple of choices here... I can leave this to you to update the pull request to make the additional changes, or we (the Aikau team) can take ownership of this issue and fix in a future sprint.

@AFaust
Copy link
Contributor Author

AFaust commented Mar 30, 2016

I'll see if I can do something about it during the weekend (when I am bored either from BeeCon preparations or liberating changes for the other ticket. In the last two days I have found some other instances of this pubSubScope prefixing that I might check out in terms of changing.

@draperd
Copy link

draperd commented Mar 31, 2016

OK, great - we'll leave this with you for the time being. If you change your mind (or find you don't have the time) then update the ticket and we'll take ownership of it.

@AFaust
Copy link
Contributor Author

AFaust commented Apr 11, 2016

I haven't yet gotten around to running the unit test suite locally. Somehow my setup using the maven-frontend-plugin to encapsulate all the node handling no longer works (intern fails on Windows), and I have yet to set up an isolated VM image for quarantining anything node related away from my normal work environment. (I focussed on #958 in the last days)

@draperd
Copy link

draperd commented Apr 11, 2016

No problem. Always worth making sure you run npm install if you're hitting issues with the testing environment as we do make updates from time to time.

@AFaust
Copy link
Contributor Author

AFaust commented May 9, 2016

Finally set up a testing VM yesterday and encountered some issues that limit any attempt at running the full test suite. Attempt to run code coverage report ended in Node spending 100% CPU for over an hour (interupting the grunt process does not kill Node), tests show some test cases not loading their test page (and failing as a result, with no correlation to the actual pubSubScope change) and tests failing with heap space errors unless POM is changed to set Xmx. I will try to run targeted/restricted tests sometime this week...

@draperd
Copy link

draperd commented May 9, 2016

Fyi... Martin and I won't be working on Aikau this week and there will be no release. Communication will be limited or non-existent. Normal service will resume next week!

@draperd
Copy link

draperd commented Dec 7, 2016

I'm going to close this as it can't be merged and realistically I couldn't merge as it was because of the unit test failures. I don't disagree with the problem though and we still have the issue that you raised to track this.

@draperd draperd closed this Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AlfList adds redundant pubSubScope prefix to alfResponseTopic for data load
2 participants