-
Notifications
You must be signed in to change notification settings - Fork 1
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
Issue78 list secondary services #79
Conversation
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.
Already looks good!
did a first pass of some review notes
|
||
TEST_USER = "Mr.Test" |
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.
Not sure yet, but I'm wondering if adding these auth related constants is indication that the tests that use these better belong under test_views.
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.
Good point.
In fact, the corresponding parts of test_views.py and test_backend.py are already testing very similar scenarios from two different points of view, one time directly on the AggregatorSecondaryServices and then again in the views using HTTP requests. Maybe that is useful, but it could just as well be too much duplication.
The main difficulty that I see here to be able to avoid extra auth setup in test_backend.py is that in the AggregatorSecondaryServices we are basically proxying the other services via BackendConnections. We are not directly using other instances of SecondaryServices, in which case we could simply pass the user_id argument to the other instance.
As a result I had to use the Flask request to pass on the authentication info to the backend connection.
Maybe there is a better way to do that, but with this method of passing authentication to the backend connection I haven't found a better way to set up authentication in the tests. Essentially it needs a request context so that authentication headers can be passed to the connection.
…ot supposed to be added to AggregatorBackendImplementation
Initial implementation of Secondary Services in the aggregator
This implements issue #78
First implement the following functionality: