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

Issue78 list secondary services #79

Closed
wants to merge 28 commits into from

Conversation

JohanKJSchreurs
Copy link
Contributor

@JohanKJSchreurs JohanKJSchreurs commented Nov 2, 2022

Initial implementation of Secondary Services in the aggregator

This implements issue #78

First implement the following functionality:

  • Listing service types
  • List services for the current/logged in user.
  • Create, update and delete a new service.

Copy link
Member

@soxofaan soxofaan left a 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"
Copy link
Member

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.

Copy link
Contributor Author

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.

@JohanKJSchreurs JohanKJSchreurs marked this pull request as ready for review November 15, 2022 08:43
@soxofaan
Copy link
Member

soxofaan commented Nov 24, 2022

Merged this PR manually (after rebase) in 39d0b89.
great work, thanks.

Follow-up issues and discussion will go to #78

@soxofaan soxofaan closed this Nov 24, 2022
@soxofaan soxofaan deleted the issue78-list-secondary-services branch November 24, 2022 12:32
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.

2 participants