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

Refactor: Change unlimited capacity concept from 0 to null #7291

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

pauloiankoski
Copy link
Contributor

@pauloiankoski pauloiankoski commented Mar 12, 2024

Resolves GIVE-462 and GIVE-484

Description

Before this pull request, we used to save 0 for the capacity field if left empty. Moving forward, we will store it as null.

This PR includes many changes to support nullable capacity:

  • TicketType Model now verifies null before converting to an integer.
  • TicketTypeRepository no longer requires capacity for validation during insert/update.
  • Create/Update Ticket Type routes now accept null or an integer for capacity, without requiring the argument.
  • SalesCount and SalesAmount columns on Events table now display "Unlimited" if any ticket type has a nullable capacity.
  • Ticket Type edit form now preserves the capacity value without attempting to convert it.
  • Ticket Type table on Event details page now handles null to display "Unlimited."
  • Ticket Type data being passed to the field template will now return either null or a number for the ticketsAvailable prop.
  • Field template checks for null or a number greater than 0 for ticketsAvailable before showing the quantity input; otherwise, it displays a simple "Sold Out" label.
  • Field template now only shows remaining tickets when the ticket type's capacity is not unlimited.

I have appended the "quantity per ticket in the donation summary" change to this pull request as it is a simple and standalone modification.

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

Comment on lines +67 to +70
return $value === 'null' ? null : absint($value);
},
'validate_callback' => function ($value) {
return $value === 'null' || rest_is_integer($value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the value be a string of 'null'?

Comment on lines +69 to +78
if ( ! isset($maxCapacitySales)) {
$maxCapacitySales = array_reduce($ticketTypes, function (Money $carry, $ticketType) {
return $carry->add($ticketType['price']->multiply($ticketType['capacity']));
}, new Money(0, give_get_currency()));

$salesPercentage = $maxCapacitySales->formatToMinorAmount() > 0 ? max(
min($salesTotal->formatToMinorAmount() / $maxCapacitySales->formatToMinorAmount(), 100),
0
) : 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the type of logic that makes me nervous without a unit test 😅

Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

Is such a large refactor required to support Unlimited Capacity? I thought it was just a bug to fix to support 0 as Unlimited? I'm not sure we need to refactor to using null, which comes with its own set of problems.

@kjohnson
Copy link
Member

I have appended the "quantity per ticket in the donation summary" change to this pull request as it is a simple and standalone modification

While this is stand alone and simple, I think it still should be submitted separately. A separate PR would associate 1:1 with a Jira ticket and could quickly be reviewed, approved, and merged.

This is particularly beneficial when the PR to which it is attached is sizable, as it requires the reviewer the separate the context and review multiple changes.

@jonwaldstein
Copy link
Contributor

Call summary (Jon & Paulo)

  • This PR introduces a concept of unlimited capacity that was not complete before
  • We dicussed nuances to having an unlimited capacity and we think a better direction would be a new boolean column specifically for this that overrides the capacity value like capacityUnlimited
  • We will circle back to this PR when we have the time
  • For now we will create a new PR that simply requires a capacity value

@pauloiankoski pauloiankoski marked this pull request as draft March 12, 2024 15:09
@kjohnson
Copy link
Member

No discussion here? I understand a call can be efficient, but the discussion started here as text.

What is wrong with using 0 as the flag for unlimited, which is fairly common practice for this sort of feature.

@jonwaldstein
Copy link
Contributor

@kjohnson the call summary is where we left the discussion, its not a definitive direction and could use more time dedication that we simply don't have at this moment.

Base automatically changed from epic/events-GIVE-265 to develop March 13, 2024 18:17
Copy link

This PR is stale because it has been open 45 days with no activity. Stale PRs will NOT be automatically closed.

@github-actions github-actions bot added the Stale label May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants