Skip to content

Conversation

@heryandjaruma
Copy link
Collaborator

@heryandjaruma heryandjaruma commented May 23, 2025

User description

  • Improve application validation
  • Fix incorrect collection in session login

PR Type

Bug fix, Enhancement


Description

  • Refactor validation functions: skip optional empties

  • Add typed error arrays and improved messages

  • Restrict file path rewrite to non-empty uploads

  • Configure Firestore ignoreUndefinedProperties

  • Fix sessionLogin user lookup collection


Changes walkthrough 📝

Relevant files
Configuration changes
firebase.ts
Configure Firestore to ignore undefined props                       

functions/src/config/firebase.ts

  • Added db.settings({ ignoreUndefinedProperties: true })
  • Enables Firestore to ignore undefined properties
  • +1/-0     
    Enhancement
    application_controller.ts
    Enhance validation logic and error handling                           

    functions/src/controllers/application_controller.ts

  • Restrict file path rewriting to non-empty values
  • Initialize typed errors arrays in all validators
  • Add early return for optional empty fields
  • Correct validation messages for number bounds
  • +53/-9   
    Bug fix
    auth_controller.ts
    Fix sessionLogin user document lookup                                       

    functions/src/controllers/auth_controller.ts

  • Change Firestore collection from "questions" to "users"
  • Fix incorrect user document lookup in sessionLogin
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @heryandjaruma heryandjaruma self-assigned this May 23, 2025
    @heryandjaruma heryandjaruma merged commit 35287e3 into main May 23, 2025
    1 check passed
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Numeric bounds check

    The validateNumberValue function uses if (validation.minValue && …) and if (validation.maxValue && …) which will skip validation when a bound is zero. Use explicit !== undefined or != null checks to ensure zero is handled correctly.

    if (validation.minValue && fieldValue < validation.minValue) {
      errors.push({
        field_id: `${question.id}`,
        message: `Must be more than or equal to ${validation.minValue}`,
      });
    }
    if (validation.maxValue && fieldValue > validation.maxValue) {
      errors.push({
        field_id: `${question.id}`,
        message: `Must be less than or equal to ${validation.maxValue}`,
      });
    String length validation

    validateStringValue compares raw string values to numeric minValue/maxValue, but should validate string length (e.g. minLength/maxLength). Otherwise non‐numeric strings will yield incorrect comparisons.

    function validateStringValue(fieldValue: string | any, question: Question) {
      const errors: { field_id: string; message: string; }[] = [];
    
      const validation = question.validation as StringValidation;
    
      // skip validation if not required and value is empty
      if (
        validation.required !== true &&
        (fieldValue === undefined || fieldValue === "" || fieldValue === null)
      ) {
    Data assignment gap

    In constructDataToSave, the new file-path rewrite is only inside a FILE type check. Verify that non-file fields are still being assigned to dataToSave as before.

    // rewrite file path
    if (question.type === QUESTION_TYPE.FILE && !(fieldValue === undefined || fieldValue === "" || fieldValue === null)) {
      dataToSave[
        question.id
      ] = `${STORAGE_BASE_LINK}${USER_UPLOAD_PATH}${UID}_${
        question.id

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate zero boundaries correctly

    The current checks skip zero boundary values because 0 is falsy. Use explicit
    undefined checks to ensure minValue or maxValue of zero are validated correctly.

    functions/src/controllers/application_controller.ts [444-455]

    -if (validation.minValue && fieldValue < validation.minValue) {
    +if (validation.minValue !== undefined && fieldValue < validation.minValue) {
       errors.push({
         field_id: `${question.id}`,
         message: `Must be more than or equal to ${validation.minValue}`,
       });
     }
    -if (validation.maxValue && fieldValue > validation.maxValue) {
    +if (validation.maxValue !== undefined && fieldValue > validation.maxValue) {
       errors.push({
         field_id: `${question.id}`,
         message: `Must be less than or equal to ${validation.maxValue}`,
       });
     }
    Suggestion importance[1-10]: 8

    __

    Why: The current checks skip zero values because 0 is falsy; checking against undefined fixes a logic bug in number validation.

    Medium
    General
    Extract empty check helper

    Extract the empty‐check logic into a reusable helper to reduce duplication and
    improve readability across multiple validators.

    functions/src/controllers/application_controller.ts [283-289]

    +function isEmpty(value: any): boolean {
    +  return value === undefined || value === null || value === "";
    +}
     // skip validation if not required and value is empty
    -if (
    -  validation.required !== true &&
    -  (fieldValue === undefined || fieldValue === "" || fieldValue === null)
    -) {
    +if (!validation.required && isEmpty(fieldValue)) {
       return errors;
     }
    Suggestion importance[1-10]: 5

    __

    Why: Extracting the repeated empty‐check logic into isEmpty improves readability and reduces duplication, but is a non‐critical refactor.

    Low

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

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants