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

Add scim package #187

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add scim package #187

wants to merge 3 commits into from

Conversation

rhysyngsun
Copy link
Collaborator

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

  • Desktop screenshots
  • Mobile width screenshots

How can this be tested?

Additional Context

@rhysyngsun rhysyngsun force-pushed the nl/add-ol-django-scim branch from 2c79d3b to fd750c7 Compare February 25, 2025 19:51
@rhysyngsun
Copy link
Collaborator Author

/gemini review

Copy link

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@rhysyngsun
Copy link
Collaborator Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request introduces the mitol-django-scim package, which integrates SCIM functionality into the Django project. This allows for user management through the SCIM protocol, which is essential for synchronizing user data with external identity providers like Keycloak. The implementation appears comprehensive, including custom adapters, parsers, and views to handle SCIM requests. However, there are several areas where improvements can be made to enhance code clarity, maintainability, and adherence to best practices.

Summary of Findings

  • Type Hinting: In several files, the type hinting for lists and dictionaries uses typing.List and typing.Dict, which are deprecated in Python 3.9 and later. It's recommended to use the built-in list and dict instead.
  • Unnecessary noqa comments: Several noqa comments (e.g., FA100, FBT002) are used without a clear reason. These should be removed if they are no longer necessary or explained if they are.
  • Code Style and Consistency: There are inconsistencies in code style, such as the use of dict | list instead of Union[dict, list] in some places. Consistent code style improves readability and maintainability.
  • Documentation and Comments: While the code includes docstrings, some areas could benefit from more detailed explanations, especially in complex logic or custom adapters.

Assessment

The pull request is a significant step towards integrating SCIM functionality into the Django project. However, before merging, it's crucial to address the identified issues related to type hinting, unnecessary noqa comments, code style inconsistencies, and documentation. Addressing these points will improve the code's quality, maintainability, and adherence to best practices. I recommend addressing these comments before requesting a review from someone else, but feel free to request another review from Gemini via /gemini review when you have addressed these comments and I'll take another look! Note that in any circumstance, users should have others review and approve this code before merging.

@rhysyngsun
Copy link
Collaborator Author

@gemini-code-assist how do I fix the failing tests?

Copy link

It looks like the tests are failing because the .secrets.baseline file needs to be updated to reflect the changes in the .github/workflows/ci.yml file. Specifically, the line_number for the hashed secret in .secrets.baseline is incorrect. Additionally, the Ruff configuration has been updated to target Python 3.10, so you may need to update any dependencies or code that are not compatible with this version.

To fix the failing tests, you can try the following:

  1. Update the .secrets.baseline file by running detect-secrets --update .secrets.baseline locally, and then commit the changes.
  2. Ensure that all dependencies are compatible with Python 3.10. If not, update the dependencies or modify the code to be compatible.
  3. Run the tests locally to ensure that they pass before committing the changes.

@rhysyngsun
Copy link
Collaborator Author

@gemini-code-assist how do I fix this error in this PR's pytest?

 _______________ ERROR collecting tests/mitol/scim/test_parser.py _______________
ImportError while importing test module '/home/runner/work/ol-django/ol-django/tests/mitol/scim/test_parser.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
E   ModuleNotFoundError: No module named 'scim.test_parser'

Copy link

The error ModuleNotFoundError: No module named 'scim.test_parser' suggests that the tests/mitol/scim/test_parser.py file is not being recognized as a module. This can happen due to a few reasons:

  1. Missing __init__.py: Ensure that the mitol and scim directories (if scim is indeed a subpackage) each contain an __init__.py file. This file can be empty, but its presence is what tells Python to treat the directory as a package.
  2. Incorrect import path: Double-check how you're trying to import the module in your test file. The traceback indicates Python is looking for scim.test_parser, implying that tests/mitol/scim/test_parser.py should be importable as such. If scim is meant to be a top-level package, the import statement in tests/mitol/scim/test_parser.py should reflect that.
  3. PYTHONPATH issues: It's possible that the project's root directory is not in Python's search path. While pytest usually handles this, it's worth verifying that the project root is included in PYTHONPATH.
  4. Typo in module or package name: Verify that there are no typos in the module or package names.

To address this, I suggest the following:

  1. Add __init__.py files to the mitol and mitol/scim directories if they don't exist.
  2. Modify the import statement in tests/mitol/scim/test_parser.py to be from mitol.scim import test_parser.
  3. Run the tests with the -v flag (e.g., pytest -v tests/mitol/scim/test_parser.py) to get more verbose output, which might provide additional clues.

@rhysyngsun rhysyngsun force-pushed the nl/add-ol-django-scim branch from 2f6ae25 to adb5e7e Compare February 27, 2025 13:01
@rhysyngsun rhysyngsun force-pushed the nl/add-ol-django-scim branch from aabc5ef to aa11089 Compare February 27, 2025 14:26
@rhysyngsun rhysyngsun force-pushed the nl/add-ol-django-scim branch from aa11089 to ed57c20 Compare February 27, 2025 14:27
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.

1 participant