Skip to content

#1788 Add mobile number column to system imports for supervisors#3335

Merged
harsohailB merged 11 commits into
rubyforgood:mainfrom
harsohailB:feature/1788
Apr 4, 2022
Merged

#1788 Add mobile number column to system imports for supervisors#3335
harsohailB merged 11 commits into
rubyforgood:mainfrom
harsohailB:feature/1788

Conversation

@harsohailB
Copy link
Copy Markdown
Collaborator

What github issue is this PR for, if any?

Resolves #1788

What changed, and why?

  • User data model to store phone number as a string
  • supervisor_importer to include new column for phone numbers and validating the inputs in each row
  • Seeding of data to include random generation of phone numbers for users using Faker

How will this affect user permissions?

  • Volunteer permissions: N/A
  • Supervisor permissions: N/A
  • Admin permissions: N/A

How is this tested? (please write tests!) 💖💪

  • Added phone numbers to existing supervisor data csv files
  • Created new test csv test files for invalid inputs and missing phone numbers
  • Created new tests in the supervisor importer spec file

Screenshots please :)

Valid Input:
Screen Shot 2022-04-02 at 9 01 49 PM

Screen Shot 2022-04-02 at 9 02 15 PM

Invalid Input:

  • Input
    Screen Shot 2022-04-02 at 8 56 51 PM

  • Error File
    Screen Shot 2022-04-02 at 8 57 13 PM

@harsohailB harsohailB added zzz_archived: 📱 SMS work relating to SMS notifications #1017 codethechange code.the.change developers labels Apr 3, 2022
@harsohailB harsohailB requested review from compwron and xihai01 April 3, 2022 03:04
@harsohailB harsohailB marked this pull request as draft April 3, 2022 04:10

if supervisor_params.key?(:phone_number)
if supervisor_params[:phone_number].length != VALID_PHONE_NUMBER_LENGTH || !supervisor_params[:phone_number].scan(/\D/).empty?
raise "Phone number is not in correct format"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify correct format in error

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as "Phone number is not in correct format: 1XXXXXXXXXX"!

Copy link
Copy Markdown
Collaborator

@compwron compwron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@compwron
Copy link
Copy Markdown
Collaborator

compwron commented Apr 3, 2022

Some strange tests failing, I'm not sure what could have gone wrong there...


rspec './spec/datatables/reimbursement_datatable_spec.rb[1:2:5:1:1]' # ReimbursementDatatable multiple record handling order by case number behaves like a sorted results set should order ascending by default
rspec './spec/datatables/reimbursement_datatable_spec.rb[1:2:5:1:3:1]' # ReimbursementDatatable multiple record handling order by case number behaves like a sorted results set descending order should order correctly
rspec './spec/datatables/reimbursement_datatable_spec.rb[1:2:5:1:2:1]' # ReimbursementDatatable multiple record handling order by case number behaves like a sorted results set explicit ascending order should order correctly

Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your supervisor_import.rb file, I see that you are making validations for the phone number. But would it be better to handle these validations in the User model instead? Since volunteer, supervisor and admin all inherit from the User model and each of these users will need the same validations for phone numbers too.

https://guides.rubyonrails.org/active_record_validations.html#length

@harsohailB
Copy link
Copy Markdown
Collaborator Author

harsohailB commented Apr 3, 2022

But would it be better to handle these validations in the User model instead?

@xihai01 Yes, this is a great point. I will make this change which will also help us bring the supervisor-importer function below the limit of 25 lines.

@harsohailB
Copy link
Copy Markdown
Collaborator Author

harsohailB commented Apr 3, 2022

Some strange tests failing, I'm not sure what could have gone wrong there...

@compwron Yea not really sure what's happening here either as the rspec / rspec (pull_request check is passing.

@harsohailB harsohailB marked this pull request as ready for review April 3, 2022 17:45
@harsohailB harsohailB requested review from compwron and xihai01 April 3, 2022 17:46
Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me and smart use of custom validators.

@harsohailB harsohailB merged commit 1f043ac into rubyforgood:main Apr 4, 2022
@FireLemons
Copy link
Copy Markdown
Collaborator

🥇

@compwron compwron added the 🧪 Tests Tests label May 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codethechange code.the.change developers ruby Touches Ruby code 🧪 Tests Tests zzz_archived: 📱 SMS work relating to SMS notifications #1017

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add mobile number column to system imports for supervisors

4 participants