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.

Sign up for GitHub

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

Jump to bottom

feat: add required fields validation logic #250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: add required fields validation logic #250

wants to merge 2 commits into from
+1,176 −155

Conversation

andrey-canon
Copy link
Collaborator

Description

This PR adds a method for validating user fields based on the REQUIRED_USER_FIELDS setting.
The validation ensures that each field meets the specified constraints, such as max_length, char_type, format, and optional_values.

Changes Introduced

Implemented validate_required_user_fields, which checks user fields against predefined validation rules.
Added specific validation methods for max_length, char_type, format, and optional_values.
Integrated structured settings to define required fields.
Added unit tests to ensure correct validation behavior.

How to Test

  1. Ensure the following setting is configured
REQUIRED_USER_FIELDS = {
    "account": {
        "first_name": {"max_length": 30, "char_type": "latin"},
        "last_name": {"max_length": 50, "char_type": "latin"},
    },
    "profile": {
        "city": {"max_length": 32, "char_type": "latin"},
        "country": {"max_length": 2, "optional_values": ["US", "CA", "MX", "BR"]},
        "phone_number": {"max_length": 15, "format": "phone"},
        "mailing_address": {"max_length": 40},
    },
    "extra_info": {
        "arabic_name": {"max_length": 20, "char_type": "arabic"},
        "arabic_first_name": {"max_length": 20, "char_type": "arabic"},
        "arabic_last_name": {"max_length": 50, "char_type": "arabic"},
    },
}
  1. Open a Django shell session:
    ./manage.py lms shell
  2. Run the validation manually:
from eox_nelp.user_profile.required_fields_validation import validate_required_user_fields
from django.contrib.auth import get_user_model

User = get_user_model()
user = User.objects.get(username="your username")

validate_required_user_fields(user)
  1. Modify the user fields in the Admin Panel
  • Change values to invalid inputs (e.g., exceeding max_length, using incorrect char_type, or entering an invalid format).
  • Run the validation again and verify that invalid fields are correctly identified.

Expected Outcome

Fields that do not comply with the settings should be flagged as invalid.
The function should return a dictionary containing invalid fields and their respective errors.

@github-actions GitHub Actions
Copy link

This PR exceeds the recommended size of 1000 lines. Check if you are NOT addressing multiple issues with one PR. If is not the case continue the review process.

@andrey-canon andrey-canon mentioned this pull request Feb 24, 2025
Closed
@andrey-canon andrey-canon force-pushed the and/add_user_fields_validation branch from 631c58f to 7896998 Compare February 24, 2025 18:13
@github-actions GitHub Actions
Copy link

This PR exceeds the recommended size of 1000 lines. Check if you are NOT addressing multiple issues with one PR. If is not the case continue the review process.

johanseto
johanseto reviewed Feb 24, 2025
View reviewed changes
Copy link
Collaborator

@johanseto johanseto left a comment

Choose a reason for hiding this comment

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

I am thinking maybe eox-nelp would need a common validators file for all the project.

Not using different validators for required_fields_validation or pearson_vue or course_experience...

This mainly for the validate_{} functions...

eox_nelp/user_profile/required_fields_validation.py Outdated
return re.match(arabic_regex, value) is not None


def validate_cp1252(value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function already exists.
https://github.com/eduNEXT/eox-nelp/blob/master/eox_nelp/pearson_vue/utils.py#L53

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/eduNEXT/eox-nelp/blob/master/eox_nelp/pearson_vue/data_classes.py#L10

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both files are removed in this pr

eox_nelp/user_profile/required_fields_validation.py Outdated
Returns:
bool: True if valid, False otherwise.
"""
arabic_regex = r'^[\u0600-\u06FF\s]+$' # Unicode range for Arabic characters
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be imported from here?
https://github.com/nelc/nelp-custom-registration-fields/blob/main/custom_reg_form/forms.py#L26

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope, basically that is a protected method

@andrey-canon
Copy link
Collaborator Author

would need a common validators file for all the project.

Not using different validators for required_fields_validation or pearson_vue or course_experience...

Is the suggestion to rename the file ? if so, what are your suggestions

@johanseto
Copy link
Collaborator

would need a common validators file for all the project.
Not using different validators for required_fields_validation or pearson_vue or course_experience...

Is the suggestion to rename the file ? if so, what are your suggestions

IMO we could use a validator.py file in the common folder of projects
https://github.com/eduNEXT/eox-nelp/tree/789699875d78288063f217c22b98d428b42ccb78/eox_nelp

Including:

  • validate_latin
  • validate_arabic
  • validate_cp1252
  • validate_email
  • validate_phone
  • validate_numeric
    These are functions with one argument(str) and return a boolean.

This is with the intention of reusing it in the rest of the folders with apps.

@andrey-canon
Copy link
Collaborator Author

would need a common validators file for all the project.
Not using different validators for required_fields_validation or pearson_vue or course_experience...

Is the suggestion to rename the file ? if so, what are your suggestions

IMO we could use a validator.py file in the common folder of projects https://github.com/eduNEXT/eox-nelp/tree/789699875d78288063f217c22b98d428b42ccb78/eox_nelp

Including:

  • validate_latin
  • validate_arabic
  • validate_cp1252
  • validate_email
  • validate_phone
  • validate_numeric
    These are functions with one argument(str) and return a boolean.

This is with the intention of reusing it in the rest of the folders with apps.

Done

@github-actions GitHub Actions
Copy link

This PR exceeds the recommended size of 1000 lines. Check if you are NOT addressing multiple issues with one PR. If is not the case continue the review process.

@andrey-canon andrey-canon force-pushed the and/add_user_fields_validation branch from 7e789ef to f5daa81 Compare February 25, 2025 22:01
@github-actions GitHub Actions
Copy link

This PR exceeds the recommended size of 1000 lines. Check if you are NOT addressing multiple issues with one PR. If is not the case continue the review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanseto johanseto Awaiting requested review from johanseto

Assignees
No one assigned
Labels
django_plugin requirements size/xl test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrey-canon @johanseto