Drop gender and birthdate columns from User

Test Plan: -specs should pass flag=none fixes VICE-591 Change-Id: I20595e1f7aefe8e4c6237db60b3eac9139780fef Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/242087 Tested-by: Service Cloud Jenkins <[email protected]> Reviewed-by: Rob Orton <[email protected]> QA-Review: Rob Orton <[email protected]> Product-Review: Rob Orton <[email protected]>
ralacas · Jul 20, 2020 · 079860e · 079860e
1 parent 0e96dba
commit 079860e
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 43 deletions.
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
@@ -1386,9 +1386,6 @@ def new
   #   The user's preferred language, from the list of languages Canvas supports.
   #   This is in RFC-5646 format.
   #
-  # @argument user[birthdate] [Date]
-  #   The user's birth date.
-  #
   # @argument user[terms_of_use] [Boolean]
   #   Whether the user accepts the terms of use. Required if this is a
   #   self-registration and this canvas instance requires users to accept
@@ -1522,9 +1519,6 @@ def create
   #   The user's preferred language, from the list of languages Canvas supports.
   #   This is in RFC-5646 format.
   #
-  # @argument user[birthdate] [Date]
-  #   The user's birth date.
-  #
   # @argument user[terms_of_use] [Required, Boolean]
   #   Whether the user accepts the terms of use.
   #
@@ -1892,10 +1886,13 @@ def update
 
     update_email = @user.grants_right?(@current_user, :manage_user_details) && user_params[:email]
     managed_attributes = []
-    managed_attributes.concat [:name, :short_name, :sortable_name, :birthdate] if @user.grants_right?(@current_user, :rename)
+    managed_attributes.concat [:name, :short_name, :sortable_name] if @user.grants_right?(@current_user, :rename)
     managed_attributes << :terms_of_use if @user == (@real_current_user || @current_user)
     managed_attributes << :email if update_email
 
+    # we dropped birthdate from user but this will allow backwards compatability and prevent errors
+    user_params.delete("birthdate")
+
     if @domain_root_account.enable_profiles?
       managed_attributes << :bio if @user.grants_right?(@current_user, :manage_user_details)
       managed_attributes << :title if @user.grants_right?(@current_user, :rename)
@@ -1951,12 +1948,6 @@ def update
         @user.require_acceptance_of_terms = true
       end
 
-      if user_params[:birthdate].present? && user_params[:birthdate] !~ Api::ISO8601_REGEX &&
-          params[:user][:birthdate] !~ Api::DATE_REGEX
-        return render(:json => {:errors => {:birthdate => t(:birthdate_invalid,
-          'Invalid date or invalid datetime for birthdate')}}, :status => 400)
-      end
-
       @user.sortable_name_explicitly_set = user_params[:sortable_name].present?
 
       respond_to do |format|
@@ -2795,19 +2786,14 @@ def create_user
 
     if params[:user]
       user_params = params[:user].
-        permit(:name, :short_name, :sortable_name, :time_zone, :show_user_services, :gender,
-          :avatar_image, :subscribe_to_emails, :locale, :bio, :birthdate, :terms_of_use,
+        permit(:name, :short_name, :sortable_name, :time_zone, :show_user_services,
+          :avatar_image, :subscribe_to_emails, :locale, :bio, :terms_of_use,
           :self_enrollment_code, :initial_enrollment_type)
       if self_enrollment && user_params[:self_enrollment_code]
         user_params[:self_enrollment_code].strip!
       else
         user_params.delete(:self_enrollment_code)
       end
-      if user_params[:birthdate].present? && user_params[:birthdate] !~ Api::ISO8601_REGEX &&
-          user_params[:birthdate] !~ Api::DATE_REGEX
-        return render(:json => {:errors => {:birthdate => t(:birthdate_invalid,
-                                                            'Invalid date or invalid datetime for birthdate')}}, :status => 400)
-      end
 
       @user.attributes = user_params
       accepted_terms = params[:user].delete(:terms_of_use)

diff --git a/app/models/user.rb b/app/models/user.rb
@@ -31,7 +31,8 @@ def self.sortable_name_order_by_clause(table = nil)
     best_unicode_collation_key(col)
   end
 
-  self.ignored_columns = %i[type creation_unique_id creation_sis_batch_id creation_email sis_name bio merge_to unread_inbox_items_count visibility account_pronoun_id]
+  self.ignored_columns = %i[type creation_unique_id creation_sis_batch_id creation_email
+                            sis_name bio merge_to unread_inbox_items_count visibility account_pronoun_id gender birthdate]
 
 
   include Context

diff --git a/db/migrate/20200707155842_drop_unused_user_columns.rb b/db/migrate/20200707155842_drop_unused_user_columns.rb
@@ -0,0 +1,25 @@
+#
+# Copyright (C) 2020 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+
+class DropUnusedUserColumns < ActiveRecord::Migration[5.2]
+  tag :postdeploy
+
+  def change
+    remove_column :users, :gender, :string, limit: 255
+    remove_column :users, :birthdate, :datetime
+  end
+end
diff --git a/spec/apis/v1/users_api_spec.rb b/spec/apis/v1/users_api_spec.rb
@@ -1297,13 +1297,6 @@ def create_user_skip_cc_confirm(admin_user)
         end
       end
 
-      it "should catch invalid dates before passing to the database" do
-        json = api_call(:post, "/api/v1/accounts/#{@site_admin.account.id}/users",
-                        { :controller => 'users', :action => 'create', :format => 'json', :account_id => @site_admin.account.id.to_s },
-                        { :pseudonym => { :unique_id => "[email protected]"},
-                          :user => { :name => "Test User", :birthdate => "-3587-11-20" } }, {}, {:expected_status => 400} )
-      end
-
       it "should allow site admins to create users and auto-validate communication channel" do
         create_user_skip_cc_confirm(@site_admin)
       end
@@ -1627,7 +1620,6 @@ def create_user_skip_cc_confirm(admin_user)
           'time_zone' => "Tijuana"
         })
 
-        expect(user.birthdate.to_date).to eq birthday.getutc.to_date
         expect(user.time_zone.name).to eql 'Tijuana'
       end
 
@@ -1691,20 +1683,6 @@ def create_user_skip_cc_confirm(admin_user)
         expect(user.profile.reload.title).to eq another_title
       end
 
-      it "should catch invalid dates" do
-        birthday = Time.now
-        json = api_call(:put, @path, @path_options, {
-            :user => {
-                :name => 'Tobias Funke',
-                :short_name => 'Tobias',
-                :sortable_name => 'Funke, Tobias',
-                :time_zone => 'Tijuana',
-                :birthdate => "-4000-02-01 10:20",
-                :locale => 'en'
-            }
-        }, {}, {:expected_status => 400})
-      end
-
       it "should allow updating without any params" do
         json = api_call(:put, @path, @path_options, {})
         expect(json).not_to be_nil