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

User Credential Views #323

Merged
merged 13 commits into from
Jul 1, 2024
Merged

User Credential Views #323

merged 13 commits into from
Jul 1, 2024

Conversation

pxwxnvermx
Copy link
Contributor

Ticket

Filter by Country and Credential, and Add Users:
image

Add Credential to Users:
image

@pxwxnvermx pxwxnvermx self-assigned this May 15, 2024
@pxwxnvermx pxwxnvermx marked this pull request as ready for review May 22, 2024 12:58
if users:
add_connect_users.delay(users, form.instance.id)
filter_country = form.cleaned_data["filter_country"]
filter_credential = form.cleaned_data["filter_credential"]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we only offer a choice on the select and do not let the user enter custom data.

@@ -25,10 +28,19 @@ def organization_home(request, org_slug):
if not form:
form = OrganizationChangeForm(instance=org)

credentials = connect_id_client.fetch_credentials()
if not len(list(c for c in credentials if c.name == f"Worked for {org.name}")):
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
if not len(list(c for c in credentials if c.name == f"Worked for {org.name}")):
if not any(c.name == f"Worked for {org.name}" for c in credentials):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. f309779 Thanks

def add_credential_view(request, org_slug):
org = get_object_or_404(Organization, slug=org_slug)
credentials = connect_id_client.fetch_credentials()
if not len(list(c for c in credentials if c.name == f"Worked for {org.name}")):
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
if not len(list(c for c in credentials if c.name == f"Worked for {org.name}")):
if not any(c.name == f"Worked for {org.name}" for c in credentials):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. f309779

Copy link
Collaborator

@calellowitz calellowitz left a comment

Choose a reason for hiding this comment

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

A couple small questions but seems mostly good once the merge conflicts are fixed.

@@ -25,10 +28,19 @@ def organization_home(request, org_slug):
if not form:
form = OrganizationChangeForm(instance=org)

credentials = connect_id_client.fetch_credentials()
if not any(c.name == f"Worked for {org.name}" for c in credentials):
credentials.append(Credential(name=f"Worked for {org.name}", slug="default"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does it mean that slug is always default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here in case there are no credentials present in the database. If a credential with the same name is present the default credential is not added to the list.

Copy link
Collaborator

@calellowitz calellowitz Jun 21, 2024

Choose a reason for hiding this comment

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

Is a credential added to the DB with a slug default ever or sent to connectid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, we only send the name of the credential that is to be added. The slugs are generated in ConnectID from the credential names.

org = get_object_or_404(Organization, slug=org_slug)
credentials = connect_id_client.fetch_credentials()
if not any(c.name == f"Worked for {org.name}" for c in credentials):
credentials.append(Credential(name=f"Worked for {org.name}", slug=f"Worked for {org.name}"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does the slug differ between the two places this occurs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, slugs should be an actual slug not normal text (lowercased, underscore separated, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. 9aa3e50

@pxwxnvermx pxwxnvermx merged commit 92aba34 into main Jul 1, 2024
2 checks passed
@pxwxnvermx pxwxnvermx deleted the pkv/user-creditial-views branch July 1, 2024 07:04
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.

None yet

3 participants