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

Fix: prevent donation details page fatal error #7711

Open
wants to merge 1 commit into
base: epic/campaigns
Choose a base branch
from

Conversation

jonwaldstein
Copy link
Contributor

@jonwaldstein jonwaldstein commented Feb 5, 2025

Resolves: GIVE-2137

Description

This fixes a fatal error on the donation details page. The problem was our check for the campaign details page was returning true in cases it should not be. So it was adding a campaignId to the request that was not correct.

There was also some logic in the request itself that could have prevented a fatal error by conditionally checking params and model instances.

Affects

  • The donations list table & details page.
  • The donation forms list table within a campaign page (should only show related forms)

Visuals

Original problem this fixes:

donations-fatal-error.mov

Testing Instructions

  • Create a campaign and donate using the campaign form
  • Navigate to the donations list and click on edit donation
  • You should not be presented with any fatal errors

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@jonwaldstein jonwaldstein marked this pull request as ready for review February 5, 2025 18:35
@glaubersilva glaubersilva self-requested a review February 6, 2025 16:15
Copy link
Contributor

@glaubersilva glaubersilva left a comment

Choose a reason for hiding this comment

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

@jonwaldstein Looks good to me! No fatal errors were thrown in my tests.

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