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.

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

Feature: Add User Billing Page for Updating Billing Information #46

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

petecheslock
Copy link
Contributor

This pull request introduces a feature that allows users to manage and update their billing information via a dedicated billing page. The user can modify their billing address, including the address, city, state, and zip code.

Changes Implemented:

Controllers

  • UsersController:
    • Added billing action.
    • Added update_billing action.
    • Added billing_params to permit billing-related parameters.

Views

  • billing.html.erb:
    • Created a new view for managing billing settings with a form for billing information.

Models

  • UserBilling:
    • Handles billing information related to a user.

Integration Tests

  • Added tests in test/integration/billing_address_test.rb:
    • Test for creating a new billing address.
    • Test for updating an existing billing address.

How to Test

  1. Navigate to the Billing Page:
    GET /users/:id/billing
  2. Update the Billing Information:
    PATCH /users/:id/billing
  3. Run the integration tests to validate the new functionality:
    rails test test/integration/billing_address_test.rb

Notes

  • Ensure that the user is logged in before accessing the billing page.
  • Proper validations are in place to ensure that the billing address fields are not left empty.
  • Flash messages are used to provide feedback to the user on success or failure.

Best Practices and Error Handling

  • Added error handling for the billing form to display appropriate error messages in case of a validation failure.
  • Utilized find_or_initialize_by to ensure that a user always has a billing address record even if starting from scratch.

Code Files Changed

  • app/controllers/users_controller.rb
  • app/views/users/billing.html.erb
  • app/models/user_billing.rb
  • test/integration/billing_address_test.rb

@petecheslock petecheslock force-pushed the feat/mongo-billing-feature branch 4 times, most recently from 230f07a to 3721249 Compare June 21, 2024 20:35
Copy link

github-actions bot commented Jun 21, 2024

AppMap runtime code review

Summary Status
Failed tests ✅ All tests passed
API changes 🔧 1 non-breaking
Security flaws ✅ None detected
Performance problems 🐌 1 new 🎉 1 resolved
Code anti-patterns 🚨 2 new 🎉 2 resolved
New AppMaps ⭐ 2 new minitest tests

🔄 API changes

🔧 Non-breaking changes

These changes are backwards-compatible, according to the OpenAPI specification.

  • Add path /users/{id}/billing
Detailed OpenAPI diff
--- base/openapi.yml	2024-06-16 01:20:30.000000000 +0000
+++ head/openapi.yml	2024-06-21 21:13:29.000000000 +0000
@@ -315,6 +315,36 @@
                       type: string
                     name:
                       type: string
+  /users/{id}/billing:
+    get:
+      responses:
+        '200':
+          content:
+            text/html: {}
+          description: OK
+    patch:
+      responses:
+        '302':
+          content:
+            text/html: {}
+          description: Found
+      requestBody:
+        content:
+          application/x-www-form-urlencoded:
+            schema:
+              type: object
+              properties:
+                billing:
+                  type: object
+                  properties:
+                    address:
+                      type: string
+                    city:
+                      type: string
+                    state:
+                      type: string
+                    zip_code:
+                      type: string
   /users/{id}/edit:
     get:
       responses:

Performance problems

🐌 New problems detected (1)

N plus 1 SQL query
Description

app_views_users_show_html_erb.render[287] contains 30 occurrences of SQL: SELECT "active_storage_attachments".* FROM "active_storage_attachments" WHERE "active_storage_attachments"."record_id" = ? AND "active_storage_attachments"."record_type" = ? AND "active_storage_attachments"."name" = ? LIMIT ?

Field Value
Rule n-plus-one-query
Impact domain Performance
View in AppMap
Stack trace
  • app/views/users/show.html.erb
  • vendor/bundle/ruby/3.1.0/gems/actionpack-7.0.4/lib/action_controller/metal/renderers.rb:140
  • vendor/bundle/ruby/3.1.0/gems/mongoid-9.0.0/lib/mongoid/railties/controller_runtime.rb:26
  • vendor/bundle/ruby/3.1.0/gems/mongoid-9.0.0/lib/mongoid/railties/controller_runtime.rb:20

🎉 Problems resolved (1)

N plus 1 SQL query
Description

app_views_users_show_html_erb.render[51] contains 30 occurrences of SQL: SELECT "active_storage_attachments".* FROM "active_storage_attachments" WHERE "active_storage_attachments"."record_id" = ? AND "active_storage_attachments"."record_type" = ? AND "active_storage_attachments"."name" = ? LIMIT ?

Field Value
Rule n-plus-one-query
Impact domain Performance
View in AppMap
Stack trace

Code anti-patterns

🚨 New problems detected (2)

Data update performed in GET or HEAD request
Description

Data update performed in HTTP request GET /account_activations/{id}/edit: UPDATE "users" SET "updated_at" = ?, "activated" = ? WHERE "users"."id" = ?

Field Value
Rule update-in-get-request
Impact domain Maintainability
View in AppMap
Related code changes
--- app/controllers/users_controller.rb
+++ app/controllers/users_controller.rb
@@ -62,8 +62,27 @@ class UsersController < ApplicationController
     render 'show_follow', status: :unprocessable_entity
   end
 
-  private
+  def billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+  end
+
+  def update_billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+    if @billing_address.update(billing_params)
+      flash[:success] = "Billing address updated"
+      redirect_to billing_user_path(@user)
+    else
+      render 'billing', status: :unprocessable_entity
+    end
+  end
 
+  private
+    def billing_params
+      params.require(:billing).permit(:address, :city, :state, :zip_code)
+    end
+    
     def user_params
       params.require(:user).permit(:name, :email, :password,
                                    :password_confirmation)
--- app/models/user.rb
+++ app/models/user.rb
@@ -38,6 +38,10 @@ class User < ApplicationRecord
     remember_digest
   end
 
+  def billing_address
+    UserBilling.find_by(user_id: self.id)
+  end
+
   # Returns a session token to prevent session hijacking.
   # We reuse the remember digest for convenience.
   def session_token
--- app/views/layouts/_header.html.erb
+++ app/views/layouts/_header.html.erb
@@ -23,6 +23,7 @@
             <ul id="dropdown-menu" class="dropdown-menu">
               <li><%= link_to "Profile", current_user %></li>
               <li><%= link_to "Settings", edit_user_path(current_user) %></li>
+              <li><%= link_to "Billing", billing_user_path(current_user) %></li>
               <li class="divider"></li>
               <li>
                 <%= link_to "Log out", logout_path,
Stack trace
Data update performed in GET or HEAD request
Description

Data update performed in HTTP request GET /account_activations/{id}/edit: UPDATE "users" SET "updated_at" = ?, "activated_at" = ? WHERE "users"."id" = ?

Field Value
Rule update-in-get-request
Impact domain Maintainability
View in AppMap
Related code changes
--- app/controllers/users_controller.rb
+++ app/controllers/users_controller.rb
@@ -62,8 +62,27 @@ class UsersController < ApplicationController
     render 'show_follow', status: :unprocessable_entity
   end
 
-  private
+  def billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+  end
+
+  def update_billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+    if @billing_address.update(billing_params)
+      flash[:success] = "Billing address updated"
+      redirect_to billing_user_path(@user)
+    else
+      render 'billing', status: :unprocessable_entity
+    end
+  end
 
+  private
+    def billing_params
+      params.require(:billing).permit(:address, :city, :state, :zip_code)
+    end
+    
     def user_params
       params.require(:user).permit(:name, :email, :password,
                                    :password_confirmation)
--- app/models/user.rb
+++ app/models/user.rb
@@ -38,6 +38,10 @@ class User < ApplicationRecord
     remember_digest
   end
 
+  def billing_address
+    UserBilling.find_by(user_id: self.id)
+  end
+
   # Returns a session token to prevent session hijacking.
   # We reuse the remember digest for convenience.
   def session_token
--- app/views/layouts/_header.html.erb
+++ app/views/layouts/_header.html.erb
@@ -23,6 +23,7 @@
             <ul id="dropdown-menu" class="dropdown-menu">
               <li><%= link_to "Profile", current_user %></li>
               <li><%= link_to "Settings", edit_user_path(current_user) %></li>
+              <li><%= link_to "Billing", billing_user_path(current_user) %></li>
               <li class="divider"></li>
               <li>
                 <%= link_to "Log out", logout_path,
Stack trace

🎉 Problems resolved (2)

Data update performed in GET or HEAD request
Description

Data update performed in HTTP request GET /account_activations/{id}/edit: UPDATE "users" SET "updated_at" = ?, "activated" = ? WHERE "users"."id" = ?

Field Value
Rule update-in-get-request
Impact domain Maintainability
View in AppMap
Related code changes
--- app/controllers/users_controller.rb
+++ app/controllers/users_controller.rb
@@ -62,8 +62,27 @@ class UsersController < ApplicationController
     render 'show_follow', status: :unprocessable_entity
   end
 
-  private
+  def billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+  end
+
+  def update_billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+    if @billing_address.update(billing_params)
+      flash[:success] = "Billing address updated"
+      redirect_to billing_user_path(@user)
+    else
+      render 'billing', status: :unprocessable_entity
+    end
+  end
 
+  private
+    def billing_params
+      params.require(:billing).permit(:address, :city, :state, :zip_code)
+    end
+    
     def user_params
       params.require(:user).permit(:name, :email, :password,
                                    :password_confirmation)
--- app/models/user.rb
+++ app/models/user.rb
@@ -38,6 +38,10 @@ class User < ApplicationRecord
     remember_digest
   end
 
+  def billing_address
+    UserBilling.find_by(user_id: self.id)
+  end
+
   # Returns a session token to prevent session hijacking.
   # We reuse the remember digest for convenience.
   def session_token
--- app/views/layouts/_header.html.erb
+++ app/views/layouts/_header.html.erb
@@ -23,6 +23,7 @@
             <ul id="dropdown-menu" class="dropdown-menu">
               <li><%= link_to "Profile", current_user %></li>
               <li><%= link_to "Settings", edit_user_path(current_user) %></li>
+              <li><%= link_to "Billing", billing_user_path(current_user) %></li>
               <li class="divider"></li>
               <li>
                 <%= link_to "Log out", logout_path,
Stack trace
Data update performed in GET or HEAD request
Description

Data update performed in HTTP request GET /account_activations/{id}/edit: UPDATE "users" SET "updated_at" = ?, "activated_at" = ? WHERE "users"."id" = ?

Field Value
Rule update-in-get-request
Impact domain Maintainability
View in AppMap
Related code changes
--- app/controllers/users_controller.rb
+++ app/controllers/users_controller.rb
@@ -62,8 +62,27 @@ class UsersController < ApplicationController
     render 'show_follow', status: :unprocessable_entity
   end
 
-  private
+  def billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+  end
+
+  def update_billing
+    @user = User.find(params[:id])
+    @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+    if @billing_address.update(billing_params)
+      flash[:success] = "Billing address updated"
+      redirect_to billing_user_path(@user)
+    else
+      render 'billing', status: :unprocessable_entity
+    end
+  end
 
+  private
+    def billing_params
+      params.require(:billing).permit(:address, :city, :state, :zip_code)
+    end
+    
     def user_params
       params.require(:user).permit(:name, :email, :password,
                                    :password_confirmation)
--- app/models/user.rb
+++ app/models/user.rb
@@ -38,6 +38,10 @@ class User < ApplicationRecord
     remember_digest
   end
 
+  def billing_address
+    UserBilling.find_by(user_id: self.id)
+  end
+
   # Returns a session token to prevent session hijacking.
   # We reuse the remember digest for convenience.
   def session_token
--- app/views/layouts/_header.html.erb
+++ app/views/layouts/_header.html.erb
@@ -23,6 +23,7 @@
             <ul id="dropdown-menu" class="dropdown-menu">
               <li><%= link_to "Profile", current_user %></li>
               <li><%= link_to "Settings", edit_user_path(current_user) %></li>
+              <li><%= link_to "Billing", billing_user_path(current_user) %></li>
               <li class="divider"></li>
               <li>
                 <%= link_to "Log out", logout_path,
Stack trace

⭐ New AppMaps

[minitest] Billing address create new billing address from test/integration/billing_address_test.rb:10

[minitest] Billing address update existing billing address from test/integration/billing_address_test.rb:30

@petecheslock petecheslock force-pushed the feat/mongo-billing-feature branch 2 times, most recently from d40f6a3 to 080e0fa Compare June 21, 2024 20:49
@petecheslock
Copy link
Contributor Author

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Security
Add authentication checks to the billing actions to ensure data security

To avoid potential security issues and ensure that only authenticated users can
access and modify their billing information, add user authentication checks in the
billing and update_billing actions.

app/controllers/users_controller.rb [65-79]

+before_action :authenticate_user!, only: [:billing, :update_billing]
+
 def billing
   @user = User.find(params[:id])
   @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
 end
 
 def update_billing
   @user = User.find(params[:id])
   @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
   if @billing_address.update(billing_params)
     flash[:success] = "Billing address updated"
     redirect_to billing_user_path(@user)
   else
     render 'billing', status: :unprocessable_entity
   end
 end
 
Suggestion importance[1-10]: 9

Why: Adding authentication checks is crucial for security, ensuring that only authorized users can access and modify billing information. This suggestion addresses a significant security concern.

9
Possible bug
Replace find with find_by to handle non-existent user records gracefully

Use find_by instead of find for user lookup to handle cases where the user might not
exist, thus preventing potential exceptions.

app/controllers/users_controller.rb [66]

-@user = User.find(params[:id])
+@user = User.find_by(id: params[:id])
+return render 'not_found', status: :not_found unless @user
 
Suggestion importance[1-10]: 8

Why: Using find_by instead of find prevents potential exceptions when a user is not found, improving the robustness of the code by handling edge cases more gracefully.

8
Enhancement
Add a belongs_to association to the UserBilling model for better ORM integration

Add a model association in UserBilling for User to leverage Rails' built-in ORM
capabilities for more intuitive and safer data handling.

app/models/user_billing.rb [1-13]

 class UserBilling
   include Mongoid::Document
+
+  belongs_to :user, class_name: 'User', foreign_key: 'user_id', inverse_of: :billing_address
 
   field :user_id, type: Integer
   field :address, type: String
   field :city, type: String
   field :state, type: String
   field :zip_code, type: String
 
   index({ user_id: 1 }, { unique: true, name: "user_id_index" })
 
   validates_presence_of :user_id, :address, :city, :state, :zip_code
 end
 
Suggestion importance[1-10]: 8

Why: Adding a belongs_to association leverages Rails' ORM capabilities, allowing for more intuitive and safer data handling, which is a beneficial enhancement to the model.

8
Maintainability
Refactor code to initialize @billing_address in a before_action to avoid repetition

Instead of initializing @billing_address twice in both billing and update_billing
actions, extract this to a before_action to DRY up your code.

app/controllers/users_controller.rb [67-72]

-@billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+before_action :set_billing_address, only: [:billing, :update_billing]
 
+private
+
+def set_billing_address
+  @billing_address = UserBilling.find_or_initialize_by(user_id: @user.id)
+end
+
Suggestion importance[1-10]: 7

Why: Extracting the initialization of @billing_address into a before_action reduces code duplication and enhances maintainability, making the code cleaner and easier to manage.

7

@petecheslock
Copy link
Contributor Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review

Duplicate Code
The code to find or initialize the @billing_address is duplicated in both billing and update_billing actions. Consider refactoring this into a before_action to DRY up the code.

Data Integrity
The user_id field is validated for presence but not for uniqueness in the context of billing addresses. This might lead to potential data integrity issues if not handled properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants