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

Replace _get_authorized_instances #3926

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Conversation

azime
Copy link
Contributor

@azime azime commented Jan 31, 2023

Recover instances authorized for a user with a single database call, instead of one database call for each configured instance

Copy link
Contributor

@pbench pbench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤞

@@ -297,7 +298,7 @@ def get_instances(self, name=None, lon=None, lat=None, object_id=None, api='ALL'
else:
# Requests without any coverage
# fetch all the authorized instances (free + private) using cached function has_access()
authorized_instances = self._get_authorized_instances(user, api)
authorized_instances = get_all_available_instances(user) # self._get_authorized_instances(user, api)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can remove the comment. git history is enough to remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -341,3 +342,24 @@ def regions(self, region=None, lon=None, lat=None, request_id=None):
resp_dict['region_id'] = key_region
response['regions'].append(resp_dict)
return response

def get_all_available_instances(self, user):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be cached.
As discussed with @pbench if we can remove the condition as described in

if not user.have_access_to_free_instances:

we can re-user the function authentification.get_all_available_instances or add a simplified and cached function only to be used for this purpose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! I added a cache

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

78.9% 78.9% Coverage
0.0% 0.0% Duplication

else:
return result

bdd_instances = user.get_all_available_instances()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bdd_instances = user.get_all_available_instances()
instances_from_db = user.get_all_available_instances()

@azime azime merged commit 40e5d9e into dev Feb 13, 2023
@azime azime deleted the replace_get_authorized_instances branch February 13, 2023 15:10
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.

5 participants