Skip to content

Comments

(Fix) Donations: prevent time loss, fix sorting, count pending, support multiple same user donos#5282

Open
Oha-you wants to merge 7 commits intoHDInnovations:developmentfrom
Oha-you:donation-fixes
Open

(Fix) Donations: prevent time loss, fix sorting, count pending, support multiple same user donos#5282
Oha-you wants to merge 7 commits intoHDInnovations:developmentfrom
Oha-you:donation-fixes

Conversation

@Oha-you
Copy link
Contributor

@Oha-you Oha-you commented Feb 18, 2026

  1. Using date type for donation's starts_at and ends_at times leads to up to 24 hours of lost paid time for all donors. Example:

    • Admin notices a "7 days VIP" donation at 23:59 UTC on Monday, February 16th and instantly approves it
    • starts_at value is set to 2026-02-16 without a time (00:00:00 internally); server time is also UTC
    • ends_at value is set to 2026-02-23 without a time (00:00:00 internally)
    • On February 23rd a scheduled script auto:remove_expired_donors starts at 00:00:01 and sets is_donor for this user to false:
      ->whereDoesntHave('donations', function ($query): void {
      $query->where('ends_at', '>', now());
      })->get();
    • Result: user only got 6 whole days of the VIP status, from Tuesday to Sunday
    • Note: timestamp type is chosen instead of datetime because it automatically adjusts values to the server timezone in case the admin decides to change it (not a single second will be lost)
    • Note: After this change users will get extra donor time depending on when the admin approves a donation and when the scheduled script starts. If it's undesirable, replace ->daily() in Console/Kernel.php with twiceDaily(), everySixHours(), etc.
    • donate-lost
  2. Donations should be sorted by updated_at property in sum and goal calculations. Example:

    • User sends a donation on February 28th (created_at = 2026-02-28)
    • Admin notices it on March 1st and approves it (updated_at = 2026-03-01)
    • It is counted towards the previous month in the charts and not counted towards the updated goal at all
    • Also change it for the profile view because "Latest donation" should mean "Latest approved donation" in case a user sends multiple ones and the admin approves them in a random order (this is also needed for the next fix)
  3. Correctly set start and end times for donations made by the same user when the current donation is still active

    • Also display updated_at instead of starts_at for "Latest donation date" because after this change starts_at might be in the future
    • If there's only one active donation, these values will be the same
    • donate-multi
  4. Remove some hardcoded USD and $ text from the donation modal

    • Also add currency inside the Cost label
    • Fix some grammar mistakes; "etc." should always have a dot
    • Use the same letter case in two places
  5. Add "Pending donations count" to the Staff Dashboard, same as for Applications (with an animated badge)

@Roardom
Copy link
Collaborator

Roardom commented Feb 18, 2026

It's usually best to make separate PRs, one for each commit here. Some of the changes look quick to approve, others not so much.

I don't think your code accounts for if a donation is lifetime. Can you look into this?

Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

Small thing I noticed

Comment on lines 86 to 90
if ($latestDonation === null) {
$donation->ends_at = $now->addDays($donation->package->donor_value);
} else {
$donation->ends_at = $latestDonation->ends_at->addDays($donation->package->donor_value);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if ($latestDonation === null) {
$donation->ends_at = $now->addDays($donation->package->donor_value);
} else {
$donation->ends_at = $latestDonation->ends_at->addDays($donation->package->donor_value);
}
$donation->ends_at = $donation->starts_at->addDays($donation->package->donor_value);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like this would also keep it simple. You can make the assumption that users use UTC so don't have to worry about daylight savings either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, thank you. AI also suggested to add ->copy() before ->addDays

Carbon mutates the object. Without copy(), you risk modifying starts_at

@Oha-you
Copy link
Contributor Author

Oha-you commented Feb 18, 2026

It's usually best to make separate PRs, one for each commit here. Some of the changes look quick to approve, others not so much.

I'll try to do so in future PRs, but some commits here are tied together.
It'll be great to merge all these changes after a review because the donation feature looked kinda unfinished to me.
But I can always move some of them to separate PRs. Feel free to ask.

I don't think your code accounts for if a donation is lifetime. Can you look into this?

You were right. I think I managed to cover all possible scenarios now.
Basically, once a Lifetime package is approved for a user, all future donations will also have ends_at === null.
I removed $donation->ends_at = null assignment because this value is null by default, same as starts_at.

I also added a new commit since I forgot to fix hardcoded values in the other Staff views: ef1335b

There I extended default SweetAlert2 options for b64DeletionMessage in resources/views/layout/default.blade.php by adding HTML support.
It shouldn't be less secure this way since you'll have to explicitly allow it only for certain modals via data-allow-html="true".

@Oha-you Oha-you requested a review from Roardom February 18, 2026 22:48
@Roardom
Copy link
Collaborator

Roardom commented Feb 19, 2026

Haven't had time to look it over yet, but briefly saw this:

It shouldn't be less secure this way since you'll have to explicitly allow it only for certain modals via data-allow-html="true".

This is an XSS vulnerability since the username field is user-controllable.

@Oha-you
Copy link
Contributor Author

Oha-you commented Feb 19, 2026

This is an XSS vulnerability since the username field is user-controllable.

Fair. Added escaping with e( ) helper to usernames, package and gateway names.

It indeed helped:

donate-sure

@Oha-you
Copy link
Contributor Author

Oha-you commented Feb 20, 2026

The last commit here: 47377eb
It's also an important one because some crypto addresses like XMR are long strings that don't fit into the limited input width of the modal.
Making inputs readonly and selecting their values "on focus" allows users to easily copy them with Ctrl+C or through the context menu.

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