Skip to content

Conversation

@heryandjaruma
Copy link
Collaborator

@heryandjaruma heryandjaruma commented May 22, 2025

User description

Fix some bug in validation which are:

  • If a field is not required, and field value is empty, then skip validation. Otherwise we validate.
  • Fix construct file upload path when the field is not required, and field value is empty.

PR Type

Bug fix


Description

  • Skip validation for optional empty fields

  • Restrict file path rewrite to non-empty uploads

  • Type annotate errors arrays in validators

  • Enable Firestore ignoreUndefinedProperties setting


Changes walkthrough 📝

Relevant files
Configuration changes
firebase.ts
Enable ignoring undefined Firestore properties                     

functions/src/config/firebase.ts

  • Add Firestore setting to ignore undefined properties
+1/-0     
Bug fix
application_controller.ts
Refine form validation and file upload logic                         

functions/src/controllers/application_controller.ts

  • Skip validation when optional fields are empty
  • Restrict file path rewrite to non-empty uploads
  • Add explicit error array typing in validators
  • Fix number validation message grammar
  • +61/-9   

    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 22, 2025
    @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

    Duplicate Skip Logic

    The validateFileUploaded function contains two identical checks to skip validation for optional empty fields. The second block is redundant and can be removed to reduce code duplication.

    // skip validation if not required and value is empty
    if (
      validation.required !== true &&
      (fieldValue === undefined || fieldValue === "" || fieldValue === null)
    ) {
      return errors;
    }
    
    // required
    if (
      validation.required === true &&
      (fieldValue === undefined || fieldValue === "" || fieldValue === null)
    ) {
      errors.push({
        field_id: `${question.id}`,
        message: `This field is required`,
      });
      return errors;
    }
    
    // skip validation if not required and value is empty
    if (
      validation.required !== true &&
      (fieldValue === undefined || fieldValue === "" || fieldValue === null)
    ) {
      return errors;
    }
    Numeric Boundary Checks

    Using truthy checks for validation.minValue and validation.maxValue will skip validation when bounds are zero. Consider using explicit undefined or null checks to ensure zero is handled correctly.

    // check value
    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}`,
      });
    File Path Rewrite Logic

    The new conditional wraps file path assignment for non-empty uploads only. Confirm that non-file question types and empty file fields are still populated correctly in dataToSave.

    // 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
    General
    Treat whitespace-only as empty

    Enhance the empty‐value check to trim whitespace so that strings containing only
    spaces are treated as empty and skipped when not required.

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

    -// skip validation if not required and value is empty
    +// skip validation if not required and value is empty or whitespace
     if (
       validation.required !== true &&
    -  (fieldValue === undefined || fieldValue === "" || fieldValue === null)
    +  (fieldValue == null || (typeof fieldValue === "string" && fieldValue.trim() === ""))
     ) {
       return errors;
     }
    Suggestion importance[1-10]: 6

    __

    Why: Enhancing the skip logic to trim whitespace avoids improper validation of blank strings, improving data integrity.

    Low
    Extract skip logic helper

    Extract the repeated optional‐skip logic into a shared helper (e.g.
    shouldSkipValidation(validation.required, fieldValue)) to reduce duplication and
    ensure consistency across all validators.

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

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

    __

    Why: Centralizing the optional‐skip logic into a helper reduces duplication and ensures consistency across validators.

    Low
    Simplify empty-value check

    Simplify the empty check by using != null and strict empty-string comparison to
    improve readability and reduce verbosity.

    functions/src/controllers/application_controller.ts [166]

    -if (question.type === QUESTION_TYPE.FILE && !(fieldValue === undefined || fieldValue === "" || fieldValue === null)) {
    +if (question.type === QUESTION_TYPE.FILE && fieldValue != null && fieldValue !== "") {
    Suggestion importance[1-10]: 4

    __

    Why: Using != null plus !== "" makes the condition more concise without altering its semantics.

    Low
    Possible issue
    Remove redundant skip block

    Remove the duplicate optional‐skip block immediately before the try in
    validateFileUploaded. Only the first pre-check is needed, so deleting the second
    prevents bypassing post-required validation logic.

    functions/src/controllers/application_controller.ts [303-309]

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

    __

    Why: Removing the duplicate optional‐skip block prevents redundant logic and ensures required‐field validation isn’t mistakenly bypassed.

    Low

    @heryandjaruma heryandjaruma merged commit 92e0c55 into dev May 22, 2025
    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