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

fix: Propagate app_name to DjanoSearch, fix naming #119

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

bmtcril
Copy link
Contributor

@bmtcril bmtcril commented Nov 1, 2024

Description: The app_name option should always filter on the whole DjangoSearch when present, not just the report. This fixes that. Also updates some variable and output names to be more clear about the state of the models.

@bmtcril bmtcril force-pushed the bmtcril/false_positives branch from 192f7f2 to 50ffcda Compare November 1, 2024 19:14
Copy link

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 Works great, thank you for this fix @bmtcril :)

  • I tested this by comparing the output from master with this branch for the following command:

    code_annotations django_find_annotations --config_file .pii_annotations.yml \
       --report_path /openedx/edx-platform/reports/pii \
       --app_name openedx.core.djangoapps.user_api \
       --lint --report --coverage
    

    Verified bug using master: command errors out because it fails to respect the --app_name user_api filter:

    Configured for report path: /openedx/edx-platform/reports/pii
    Configured for source path: ./
    Found safelist at .annotation_safe_list.yml. Reading.
    
    Performing linting checks...
    
    Search failed due to linting errors!
    4 errors:
    ---------------------------------
    celery_utils.FailedTask is annotated, but also in the safelist.
    /openedx/venv/lib/python3.11/site-packages/edx_proctoring/models.py::edx_proctoring.ProctoredExamSoftwareSecureReviewHistory: "to_be_implemented" is not a valid choice for ".. pii_retirement:". Expected one of ['retained', 'local_api', 'consumer_api', 'third_party'].
    /openedx/venv/lib/python3.11/site-packages/edx_proctoring/models.py::edx_proctoring.ProctoredExamSoftwareSecureReview: "to_be_implemented" is not a valid choice for ".. pii_retirement:". Expected one of ['retained', 'local_api', 'consumer_api', 'third_party'].
    /openedx/venv/lib/python3.11/site-packages/edx_proctoring/models.py::edx_proctoring.ProctoredExamSoftwareSecureComment: "to_be_implemented" is not a valid choice for ".. pii_retirement:". Expected one of ['retained', 'local_api', 'consumer_api', 'third_party'].
    

    Verified the fix using this branch:

    Configured for report path: /openedx/edx-platform/reports/pii
    Configured for source path: ./
    Found safelist at .annotation_safe_list.yml. Reading.
    
    Performing linting checks...
    Linting passed without errors.
    
    Model coverage report
    ----------------------------------------
    Found 8 total models.
    8 were eligible for annotation, 8 were annotated.
    Coverage is 100.0%
    
    Coverage passed without errors.
    
  • I read through the code

  • I checked for accessibility issues N/A

  • Includes documentation N/A

  • User-facing strings are extracted for translation N/A

The app_name option should always filter on the whole DjangoSearch when
present, not just the report. This fixes that. Also updates some
variable and output names to be more clear about the state of the
models.
@bmtcril bmtcril force-pushed the bmtcril/false_positives branch from 8a346cb to 0338f2c Compare November 14, 2024 15:59
@bmtcril bmtcril merged commit f60763c into master Nov 19, 2024
10 of 11 checks passed
@bmtcril bmtcril deleted the bmtcril/false_positives branch November 19, 2024 20:50
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.

3 participants