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

4978 rework donation site import #5079

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

Conversation

someykoku
Copy link
Contributor

Resolves #4978

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Documentation update

How Has This Been Tested?

Screenshots

@someykoku someykoku closed this Mar 7, 2025
@someykoku someykoku reopened this Mar 7, 2025
@someykoku someykoku closed this Mar 7, 2025
@someykoku someykoku deleted the rework_donation_site_import_#4978 branch March 7, 2025 16:53
@someykoku someykoku restored the rework_donation_site_import_#4978 branch March 7, 2025 17:24
@someykoku someykoku reopened this Mar 7, 2025
@cielf
Copy link
Collaborator

cielf commented Mar 8, 2025

The error that I'm getting if I try to upload a duplicate is not ideal -- can we include the name of the site that did not upload properly, please?

Image

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hi @someykoku
Thanks for this! I've done some functionality testing. This basically works well, but we need some tweaks (see above as well.) When those are addressed, I'll pass it on for final technical review.

Donation Site 2,456 Donation Site Way,Jane Smith,[email protected],234-567-8901
Donation Site 3,789 Donation Site Way,Bob Johnson,[email protected],345-678-9012
Donation Site 3,789 Donation Site Way,Bob Johnson,[email protected],345-678-9012
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not give them a duplicated donation site as an example that they should follow. (please remove the second Donation site 3 or change it up)

@@ -22,7 +22,8 @@ class DonationSite < ApplicationRecord

belongs_to :organization

validates :name, :address, presence: true
validates :name, :address, :phone, presence: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Phone is not mandatory -- this is breaking bin/setup and a lot of the sites in production have no contact information.

it { should validate_presence_of(:address) }
it { should validate_length_of(:contact_name).is_at_least(3) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that contact name is allowed to be blank.

@someykoku someykoku requested a review from cielf March 13, 2025 12:15
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.

Rework donation site import -- should have more fields, and give error when donation sites not imported.
2 participants