Skip to content

Conversation

@betsyecastro
Copy link
Contributor

Implements the following API request validation rules:

  • Person (slug): Must contain only alphanumeric characters and periods.
  • Search fields: Must be at least three characters long.
  • School: Must match one of the whitelisted school short names.
  • With data: Allowed only when the person parameter is present.

@betsyecastro betsyecastro requested a review from wunc May 13, 2025 15:49
@betsyecastro betsyecastro self-assigned this May 13, 2025
@betsyecastro betsyecastro added 🐛 bug Something isn't working ✨ enhancement New feature or request ⬆️ priority:high High priority issue labels May 13, 2025
@betsyecastro betsyecastro changed the title Improve API request validations Improve API Request Validations May 20, 2025
@betsyecastro betsyecastro marked this pull request as draft May 20, 2025 16:55
@betsyecastro betsyecastro marked this pull request as ready for review June 5, 2025 18:51
@shukla-m shukla-m self-requested a review June 9, 2025 20:23
@shukla-m
Copy link

shukla-m commented Jun 9, 2025

@betsyecastro I added myself as a reviewer on this PR, as we discussed last week, and pulled in the code to test locally. I tried the following URL which passes validation for the person and with_data fields. The person query parameter is invalid - yes I was trying to break it! 😃

http://profiles.test/api/v1?with_data=1&data_type=information&public=1&person=herve;abdi

This might not be an issue but wanted to mention, in case ProfileApiRequest.php@validateWithData() method needed to check for valid person in addition to the existing check for non-empty person. Let me know if you would like to look at it together or discuss. Thanks.

Copy link

@shukla-m shukla-m left a comment

Choose a reason for hiding this comment

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

Looks good! I tested the updates locally and it seemed to work as expected. See comment regarding person query parameter with invalid value, in case it causes any issues.

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

Labels

🐛 bug Something isn't working ✨ enhancement New feature or request ⬆️ priority:high High priority issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants