Skip to content

Transaction Reports#37

Merged
willymwai merged 3 commits intomainfrom
feat/transaction-reports
Jul 5, 2025
Merged

Transaction Reports#37
willymwai merged 3 commits intomainfrom
feat/transaction-reports

Conversation

@Pinchez25
Copy link
Contributor

@Pinchez25 Pinchez25 commented Jul 5, 2025

User description

What is the Purpose?

What was the approach?

How can the changes made get tested?

Are there any concerns to addressed further before or after merging this PR?

Mentions?

Issue(s) affected?


PR Type

Enhancement


Description

  • Update Helm chart and app versions to 0.3.34/1.17.34

  • Add PAYMENTS_GATEWAY_URL environment variable configuration

  • Update client image tag to c38058

  • Enable environment variable injection in deployment


Changes diagram

flowchart LR
  A["Chart.yaml"] --> B["Version Bump"]
  C["values.yaml"] --> D["Image Tag Update"]
  C --> E["Environment Variables"]
  F["deployment.yaml"] --> G["Env Injection"]
  E --> G
Loading

Changes walkthrough 📝

Relevant files
Configuration changes
Chart.yaml
Version bump for chart and application                                     

charts/jisort/Chart.yaml

  • Bump chart version from 0.3.33 to 0.3.34
  • Update appVersion from 1.17.33 to 1.17.34
  • +2/-2     
    deployment.yaml
    Enable environment variable configuration in deployment   

    charts/jisort/templates/deployment.yaml

  • Add environment variable injection for client container
  • Enable env values from values.yaml configuration
  • +1/-0     
    values.yaml
    Update image tag and add payments configuration                   

    charts/jisort/values.yaml

  • Update client image tag from aecbae to c38058
  • Add PAYMENTS_GATEWAY_URL environment variable
  • Configure payments gateway URL as https://payments.jisort.com
  • +4/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @willymwai
    Copy link
    Member

    /improve

    @willymwai
    Copy link
    Member

    /review

    @willymwai
    Copy link
    Member

    /describe

    @willymwai
    Copy link
    Member

    /review

    @willymwai
    Copy link
    Member

    /improve

    @qodo-code-review
    Copy link

    qodo-code-review bot commented Jul 5, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 1d0a13e)

    Here are some key observations to aid the review process:

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

    Hardcoded URL

    The PAYMENTS_GATEWAY_URL is hardcoded to a production URL which may not be appropriate for all environments. Consider making this configurable per environment or using a template variable.

    - name: PAYMENTS_GATEWAY_URL
      value: "https://payments.jisort.com"
    Missing Validation

    The environment variables are injected without validation. If Values.client.env is not defined or is malformed, this could cause deployment issues.

    env: {{- toYaml .Values.client.env | nindent 12 }}
    livenessProbe:

    @qodo-code-review
    Copy link

    Persistent review updated to latest commit 1d0a13e

    1 similar comment
    @qodo-code-review
    Copy link

    Persistent review updated to latest commit 1d0a13e

    @qodo-code-review
    Copy link

    Title

    Transaction Reports


    User description

    What is the Purpose?

    What was the approach?

    How can the changes made get tested?

    Are there any concerns to addressed further before or after merging this PR?

    Mentions?

    Issue(s) affected?


    PR Type

    Enhancement


    Description

    • Update Helm chart and app versions to 0.3.34/1.17.34

    • Add PAYMENTS_GATEWAY_URL environment variable configuration

    • Update client image tag to c38058

    • Enable environment variable injection in deployment


    Changes diagram

    flowchart LR
      A["Chart.yaml"] --> B["Version Bump"]
      C["values.yaml"] --> D["Image Tag Update"]
      C --> E["Environment Variables"]
      F["deployment.yaml"] --> G["Env Injection"]
      E --> G
    
    Loading

    Changes walkthrough 📝

    Relevant files
    Configuration changes
    Chart.yaml
    Version bump for chart and application                                     

    charts/jisort/Chart.yaml

  • Bump chart version from 0.3.33 to 0.3.34
  • Update appVersion from 1.17.33 to 1.17.34
  • +2/-2     
    deployment.yaml
    Enable environment variable configuration in deployment   

    charts/jisort/templates/deployment.yaml

  • Add environment variable injection for client container
  • Enable env values from values.yaml configuration
  • +1/-0     
    values.yaml
    Update image tag and add payments configuration                   

    charts/jisort/values.yaml

  • Update client image tag from aecbae to c38058
  • Add PAYMENTS_GATEWAY_URL environment variable
  • Configure payments gateway URL as https://payments.jisort.com
  • +4/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link

    qodo-code-review bot commented Jul 5, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 1d0a13e

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add conditional environment variable check

    Add a conditional check to prevent template rendering errors when client.env is
    not defined or empty. This prevents potential deployment failures if the
    environment variables are not configured.

    charts/jisort/templates/deployment.yaml [169]

    +{{- if .Values.client.env }}
     env: {{- toYaml .Values.client.env | nindent 12 }}
    +{{- end }}
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: This is a good practice for Helm charts; adding a conditional check for .Values.client.env makes the chart more robust against configurations where this value is not defined.

    Low
    • More

    Previous suggestions

    Suggestions up to commit 1d0a13e
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add conditional check for environment variables

    Add a conditional check to prevent template rendering errors when client.env is
    not defined. This ensures the deployment doesn't fail if the environment
    variables section is missing from values.yaml.

    charts/jisort/templates/deployment.yaml [169]

    +{{- if .Values.client.env }}
     env: {{- toYaml .Values.client.env | nindent 12 }}
    +{{- end }}
    Suggestion importance[1-10]: 7

    __

    Why: This is a valid Helm best practice that improves the chart's robustness by preventing a template rendering error if .Values.client.env is not defined by a user.

    Medium
    Suggestions up to commit 1d0a13e
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add conditional check for environment variables

    Add a conditional check to prevent template rendering errors when client.env is
    not defined. This prevents potential deployment failures if the environment
    variables section is missing from values.

    charts/jisort/templates/deployment.yaml [169]

    +{{- if .Values.client.env }}
     env: {{- toYaml .Values.client.env | nindent 12 }}
    +{{- end }}
    Suggestion importance[1-10]: 7

    __

    Why: This is a valid Helm chart best practice that improves robustness by preventing template rendering errors if .Values.client.env is not provided.

    Medium

    @qodo-code-review
    Copy link

    Persistent suggestions updated to latest commit 1d0a13e

    @willymwai willymwai merged commit 02ca851 into main Jul 5, 2025
    1 check passed
    @willymwai willymwai deleted the feat/transaction-reports branch July 5, 2025 06:00
    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