-
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
Fetch and display commune name #583
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,7 +62,7 @@ | |
"username": "e2e.marie", | ||
"email": "[email protected]", | ||
"firstName": "Marie", | ||
"lastName": "Devarzy", | ||
"lastName": "Delamairie", | ||
"enabled": true, | ||
"attributes": { | ||
"siret": "21580304000017" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -641,6 +641,8 @@ class Development(Base): | |
|
||
OIDC_ORGANIZATION_REGISTRATION_ID_FIELD = "siret" | ||
|
||
ORGANIZATION_PLUGINS = ["plugins.organizations.NameFromSiretOrganizationPlugin"] | ||
|
||
def __init__(self): | ||
"""In dev, force installs needed for Swagger API.""" | ||
# pylint: disable=invalid-name | ||
|
@@ -686,6 +688,10 @@ class Test(Base): | |
# this is a dev credentials for mail provisioning API | ||
MAIL_PROVISIONING_API_CREDENTIALS = "bGFfcmVnaWU6cGFzc3dvcmQ=" | ||
|
||
OIDC_ORGANIZATION_REGISTRATION_ID_FIELD = "siret" | ||
|
||
ORGANIZATION_PLUGINS = ["plugins.organizations.NameFromSiretOrganizationPlugin"] | ||
Comment on lines
+691
to
+693
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, this should not be enforced here, only in specific tests. |
||
|
||
ORGANIZATION_REGISTRATION_ID_VALIDATORS = [ | ||
{ | ||
"NAME": "django.core.validators.RegexValidator", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import logging | ||
|
||
import requests | ||
from requests.adapters import HTTPAdapter, Retry | ||
|
||
from core.plugins.base import BaseOrganizationPlugin | ||
|
||
|
@@ -33,20 +34,17 @@ def _extract_name_from_organization_data(organization_data): | |
logger.warning("Empty list 'liste_enseignes' in %s", organization_data) | ||
return None | ||
|
||
def _get_organization_name_from_siret(self, siret): | ||
"""Return the organization name from the SIRET.""" | ||
try: | ||
response = requests.get(self._api_url.format(siret=siret), timeout=10) | ||
response.raise_for_status() | ||
data = response.json() | ||
except requests.RequestException as exc: | ||
logger.exception("%s: Unable to fetch organization name from SIRET", exc) | ||
return None | ||
|
||
def get_organization_name_from_results(self, data, siret): | ||
"""Return the organization name from the results of a SIRET search.""" | ||
for result in data["results"]: | ||
nature = "nature_juridique" | ||
commune = nature in result and result[nature] == "7210" | ||
for organization in result["matching_etablissements"]: | ||
if organization.get("siret") == siret: | ||
return self._extract_name_from_organization_data(organization) | ||
if commune: | ||
return organization["libelle_commune"].title() | ||
|
||
return self._extract_name_from_organization_data(organization) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning, the indentation has changed here, this might get the wrong "siret". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I see how to construct a case where we'd get the wrong result here (i.e. a missing unit test). The indent is by intention: I did mean to return directly with the |
||
|
||
logger.warning("No organization name found for SIRET %s", siret) | ||
return None | ||
|
@@ -63,10 +61,21 @@ def run_after_create(self, organization): | |
|
||
# In the nominal case, there is only one registration ID because | ||
# the organization as been created from it. | ||
name = self._get_organization_name_from_siret( | ||
organization.registration_id_list[0] | ||
) | ||
if not name: | ||
try: | ||
# Retry logic as the API may be rate limited | ||
s = requests.Session() | ||
retries = Retry(total=5, backoff_factor=0.1, status_forcelist=[429]) | ||
s.mount("https://", HTTPAdapter(max_retries=retries)) | ||
|
||
siret = organization.registration_id_list[0] | ||
response = s.get(self._api_url.format(siret=siret), timeout=10) | ||
response.raise_for_status() | ||
data = response.json() | ||
name = self.get_organization_name_from_results(data, siret) | ||
if not name: | ||
return | ||
except requests.RequestException as exc: | ||
logger.exception("%s: Unable to fetch organization name from SIRET", exc) | ||
return | ||
|
||
organization.name = name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,12 @@ export const AccountDropdown = () => { | |
<DropButton | ||
button={ | ||
<Box $flex $direction="row" $align="center"> | ||
<Text $theme="primary">{userName}</Text> | ||
<Box $flex $direction="column" $align="left"> | ||
<Text $theme="primary">{userName}</Text> | ||
{userData?.organization?.registration_id_list?.at(0) && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the condition, why should it be only when there is a registration ID? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does seem indirect now you mention it, testing for the name that we're going to display would work just as well. (I think I was biased by the testing perspective, the distinguishing feature of the organization that satisfies that test is that it's the only one in our test data that has a SIRET.) |
||
<Text $theme="primary">{userData?.organization?.name}</Text> | ||
)} | ||
</Box> | ||
<Text className="material-icons" $theme="primary" aria-hidden="true"> | ||
arrow_drop_down | ||
</Text> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ backend: | |
OIDC_RP_SCOPES: "openid email siret" | ||
OIDC_REDIRECT_ALLOWED_HOSTS: https://desk-preprod.beta.numerique.gouv.fr | ||
OIDC_AUTH_REQUEST_EXTRA_PARAMS: "{'acr_values': 'eidas1'}" | ||
ORGANIZATION_PLUGINS: "plugins.organizations.NameFromSiretOrganizationPlugin" | ||
ORGANIZATION_PLUGINS: ["plugins.organizations.NameFromSiretOrganizationPlugin"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will break, django-configurations does not expect a list here: https://github.com/jazzband/django-configurations/blob/master/configurations/values.py#L214 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That one is confusing to me, originally I picked up the string value from this file and used it without change in |
||
ORGANIZATION_REGISTRATION_ID_VALIDATORS: '[{"NAME": "django.core.validators.RegexValidator", "OPTIONS": {"regex": "^[0-9]{14}$"}}]' | ||
LOGIN_REDIRECT_URL: https://desk-preprod.beta.numerique.gouv.fr | ||
LOGIN_REDIRECT_URL_FAILURE: https://desk-preprod.beta.numerique.gouv.fr | ||
|
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.
This should not be enforced here because this is not generic.
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.
General reply to your comments: thanks for the feedbacks, I've already hit "merge" but can address these in a later PR (might pick #622 which is in some ways a continuation, or a new one altogether depending on urgency).
Specific reply to the above: OK, I understand the concern here is for users and contributors outside of France to be able to start from a vanilla configuration instead of having to figure out what is French-specific (or even French territorial administration specific) stuff that needs swapping out. And I don't think this case has arisen before so it's a good occasion to start documenting that.
On the other hand I'd like to keep these active for local development and documented in the repo for ease of onboarding. Can we put these in, say,
env.d/development/france.dist
and pick them up in docker-compose.yml with a comment suggesting creating a country-specific set of env vars, as needed for other countries ?