Proposal - store metadata on membership renewal on line item

added comp:CiviMember needs:concept approval type:proposal labels

added triaged label

changed the description

I don't think the membership status is the place to hold this. If a membership is pending renewal, there is a possibility that the member will fail to send payment. In this case, we would see a lot of pending renewal memberships stacking up.

It's also ambiguous what the underlying status is. Is this an eager beaver who would otherwise have a current membership for months and months? Is this someone in a grace period who should retain some member privileges? Is this a membership that is long-expired? This is complicated by the first concern: someone might go to renew several months early, fail to send a check, and then sit in pending renewal for the next year, having passed through what would otherwise be several membership statuses.

With site functionality and member benefits tied to membership statuses, it would be difficult to grant or revoke benefits/privileges based upon this status.

Finally, it's unclear how this would interact with the override feature. If override is set, should that override pending renewal? If not, how will we know what status to restore the membership to?

I see your points about the status of the membership being clouded by this new approach. I have run into this problem previously with "status"-driven systems where all manner of statuses get introduced to accommodate things that are better tracked elsewhere.

I would suggest we take a step back and focus on what people do when they were doing this manually, without the support of Civi:

In my client's world, a pending payment implies an invoice that someone intends to pay by check. They are an odd duck, however, in that they allow folks who expire to "resurrect" them by catching up on payments. So when someone pays, my client will look at the payment and see if it covers the necessary catch-up, and if not, they'll ask for more.

For a more standard situation, I would imagine there's a time period during which that check would be welcomed, but it's not indefinite, probably a month or 90 days. This correlates to an "expired invoice". So, perhaps this indicates that pending payments should have an expiration date after which they transition to some other status.

So, what is the difference between an invoice that is paid immediately in the same moment as a request for membership renewal and one that is paid months later? In the business world, it's simply time. A paper invoice is just real-world state data, and the actions that will be carried out when that invoice is paid are understood by both parties and are the same regardless of the time taken to pay it (unless the invoice expires). This makes me think it should be the same case in the Civi code.

So, coming back to the code, then.

My main problem with adding a column to the line items table is the problem I've had with how Civi managed memberships all along: We seem to be polluting the financial subsystem with the specifics of memberships, or vice versa.

Because of that, I lean towards some sort of "payment completion" action state data getting stored which is linked to the membership component and which would be populated at the time the payment is created (but prior to its completion, EVEN in the situation where checks are entered directly, or PayPal payments are made) and then, when the transaction completes, whatever component has registered itself as responding to the payment completion action would get notified with its state data. Then it could do whatever it deems fit with the information, such as renewing a membership, making an event participant "registered", or whatever else the component's developer has dreamed up.

One nice side effect about this approach is that a single payment could be tied to multiple completion actions (one to many data records), such as both membership and event registration, without the financial code having to know a single thing about any of it.

From what I've seen in the Civi codebase in my adventure with this topic, it suffers from a strange split personality where a payment entered directly by the admin during renewal is handled in an entirely different way from pending payments that are completed later. It's no wonder we're having this problem of scenario-specific behavior when this is the case. And this, for me, points up the need for what I've described here, because there's not much difference between a payment completed when the membership is entered and one that is paid months later, apart from time -- and time simply implies the need for maintaining state.

And finally, on the topic of administrative override, I've considered that an "all bets are off" kind of thing. If you override, you take the process into your own hands and it's on you to determine what is the appropriate action, if any, regardless of payment or timing.

Instead, it might be more helpful to add a boolean field onto civicrm_membership_payment that would indicate whether the membership change is pending. When the referenced contribution is completed such as to complete the renewal, the pending flag would be switched back to 0.

This is similar to my thoughts back on PR 18618 where the number of terms associated with the contribution-membership combo could be stored there on civicrm_membership_payment.

@andrewhunt so my thinking was that we would use the status only when they would otherwise be expired and it would be something selected from the renewal form, rather than the new normal flow, when the user wants to pre-set what the dates will be when confirmed. The underlying status & dates would be determined by the last entry in the membership log table & this is what it would revert back to. I suspect Pending Renewal might itself get the is_override flag set.

The problem with a field on the membership_payment table (or the line_item table which I think is better but serves the same thing) is we have no way to record the intended preferred dates when the payment comes in. This is something you can alter via the form for real time (via some fairly sledgehammery form use)

Ah I wasn't thinking about this: you go to renew, the form could potentially expose end_date and let you pick whatever date you want. Is that what you are thinking about?

If so, that seems like more of an edge case than someone (a few days before expiration) filling the form to renew and then taking a few weeks to get the check in. I've never heard of an organization letting people pick their own membership dates.

The same principle would apply to other fields, though, so it is a consideration. One client has a field on the membership of whether the journal subscription should be delivered by mail or just online. (They have a couple of journals, so this preference is specific to the membership rather than the contact.) However, my feeling is that this stuff--anything that won't affect the status--is fine to overwrite on the membership in real time.

In saying this, I did think about membership upgrades and don't have a ready answer. If I am a regular member and then go to renew as a super-awesome member with pay-later, how do I continue with the regular member status until I actually follow through with my super-awesome dues?

@andrewhunt I'm not thinking about front end forms - so no not letting people pick their own membership dates.

I'm thinking only about the back office renewal form and a data model to represent the administrator's intent when renewing expired or expiring memberships. This would be a no-change for other forms.

The administrator currently has 2 ways to adjust the outcome to fit their intent when they have a payment in front of them

they can enter a renewal date - the text encourages them to 'make one up' to adjust the outcome - ie if they don't want start date to change
they can enter the renewal but edit the dates straight after.

But if the payment is disconnected from the act of renewing then the intent is lost.

To come back to @jptillman - their intent is the start date won't change, but the end date will be 2 years after the original expired date. Depending on how they enter the payment the start date will or won't currently change. Both variants could be seen as a 'feature' - but fundamentally the back office user knows what is right for the scenario they are dealing with and having a way to record their intent is currently missing

That makes sense. Still, I think that using the membership status will create more problems than it solves.

Memberships are unusual in CiviCRM because they're the only situation where there's a pending action and both the pre-action and post-action states matter at their respective points in time. Regular contributions and new event registrations can be ignored if they're pending--they don't represent paid-up things. Modified event registrations are different because the state matters for only one point in time: aside from making sure the pricing adds up, we don't care that you used to want the walking tour package; we care that now you want the VIP ticket and awards dinner because that's what you're going to get.

I really am not comfortable with modifying the membership record until the renewal is paid. Doing so causes data loss. Even limiting it to "expired or expiring memberships" raises the question of how to operationalize that.

Is it any membership where the end date is in the past? That won't cover the "expiring" case.
Is it any membership in a status with that problematic "current" checkbox blank? We advise clients to mark grace periods (1-12 months, depending) as "current" so renewals during that time continue the membership uninterrupted.
Is it Grace or Expired memberships? People have all kinds of membership statuses, both before and after the end date, with different names.

This just leaves me with the feeling that the only way to resolve this is to have a place to stash data about a pending renewal so it can be applied when that renewal contribution is completed.

I had felt that stashing the number of terms might have been sufficient, since it seems like an edge case even for an admin to want to modify the end date of an unpaid renewal. It strikes me as a UI problem that the form offers that field. However, I have no objection to stashing any/all data about a membership.

It would also allow us to give clear guidance to a user: if you want to modify a membership in real time, edit the membership record. If you want to tee up changes to take effect upon renewal, use the renewal form.

If I'd read this one before I replied above, I would have seen that Andrew's statement that...

This just leaves me with the feeling that the only way to resolve this is to have a place to stash data about a pending renewal so it can be applied when that renewal contribution is completed.

...fits exactly with my thoughts on "payment completion state data". I think we are converging on an "event oriented" design with some stashed state data to help the event handlers do their thing. In my experience, this has always yielded good results in workflow situations.

I'll reiterate that it should be the case for ALL transactions, pending or immediate. That will keep the behaviors consistent regardless of scenario.

I'd also like to suggest that we should consider the scope of this "payment completion action" dilemma to go beyond that of the features we are currently familiar with. Right now, for example, I'm shoe-horning a "certification tracking" function for one of my clients into the membership tracking feature. It's starting to show some seams, however, and I'm considering creating a custom extension for it. And its payment behaviors will very closely mirror those of memberships.

Let's not freeze out the extension developers who have ideas we haven't even dreamt of yet!

@andrewhunt so one thing I think has been unclear is that I ONLY envisaged the Pending Renewal being used in cases where the admin wants that extra control over the final outcome. If they just want the normal flow to happen depending on the payment date when it comes in they wouldn't check the box to put it into Pending Renewal status and it would remain expired until they do so.

If they do it's almost the equivalent of an offer - 'as long as the payment is received within x timeframe then these dates apply'.

I should also draw your attention to core#1402 (closed) - the issue is the final dates (& treatment of start date) for completing a contribution depend which form they use. You could make a case that either is right - but we shouldn't do 2 different things

I don't really understand the issue fully here but I do know that memberships are used in multiple different ways by different organisations and one set of rules generally doesn't work universally.

I don't really see a need for adding an additional membership status and would have thought that the use-case being described should be handled either by custom data or some other form of data that can be linked to the membership.

I would really like to see all membership logic pulled out into a (core) extension that could be overridden/replaced to suit an individual organisation. It would also make it much easier to see exactly how it works.

I think the approach that I and @andrewhunt each independently suggested (above) would achieve the data segregation you want. It would also solve the blocker that @eileen mentions, because the "renewal date" "number of terms", et al, could be kept with the context data stored for the final action after payment completion. It seems to cleanly cut through the knot of logic we are struggling with.

The more I mull this over, the more strongly I feel that the membership payment completion code should be the same regardless of whether payment is completed immediately or later. It just makes no sense to have the code split here, when one approach can handle both.

I agree that migrating the membership logic to a nice independent extension would be great. Having that would certainly have helped me in getting up to speed. I'm still not there yet.

@jptillman @andrewhunt so if we are looking for an existing place to store the intent it might be possible in the membership_log table. Failing that I think we would need to add a new table. I don't think we can extend the membership_payment table because the data model doesn't preclude having more than one purchase of the same membership type on different line items (buy 2 terms get one free). How that could work in practice is a bit beyond me but the data model does permit for payment matching by line item - so the entity we would want to extend is the line item - so it would be more like a table

line_item_metadata which would have a line item id & a json blob of additional data that would be used when fulfilling payment on the line item

( I note the membership log approach wouldn't have the line item vs contribution nuance but I think if we can fit it into the existing schema we can punt on it - it's only if adding to the schema I feel we need to consider that)

I'm likely the greenest one in this discussion when it comes to the codebase and its legacy issues, so I'm certainly willing to defer to the more experienced folks on this. However, I'm really intrigued by the concept of making this independent of membership. If there were a generic payment completion function which utilized the "line_item_metadata" to determine what completion action to call and passed that context data along to it, it would make the process so much more powerful and less prone to design troubles later on.

I may be just causing annoyance if there are constraints I don't know about, such as a taboo on adding new tables. Please let me know if this is so. I want to be helpful and not distracting.

@jptillman so we do have a generic payment completion function - the issue it is doesn't have all the data it needs in this case- which is our limitation

Adding to / altering the schema is not something we do lightly - but at the same time there may be a gap only a schema change will help with - @JoeMurray might also have thoughts

@eileen said:

I don't think we can extend the membership_payment table because the data model doesn't preclude having more than one purchase of the same membership type on different line items (buy 2 terms get one free).

You're right that the data model could theoretically support this, but the form logic prohibits it, and I'm not confident that the membership processing code would handle this in a way that anyone might reasonably expect.

Backend:

Frontend:

Unless and until the business logic can handle something like this, I don't think we need to dance around it in the data schema. Moreover, one natural way to resolve a contribution with two line items with the same membership type might be to resolve it at the form level and record the consolidated membership effect into civicrm_membership_payment.

But at the same time I'm not opposed to a new line_item_metadata field, and I think it could do a good job for other purposes via extension like @jptillman has suggested. In fact, this would help permanently store data that might change on the price field/option. Right now, there's no way to be sure that the membership type or number of terms on the price option are the values that were there when a contribution was recorded.

@andrewhunt so to be clear - we are talking about a new line_item_metadata field in a new table - that could store line item metadata for any line item.

My only reason for thinking a new table rather than adding to the line_item table is that the line_item table is large already & this field might not be needed for all rows - but of course if it's not indexed then that might not really matter.

The membership_payment table duplicates the line item table for linking memberships to payments & we are moving away from referring to it in favour of the line item table in the code.

@mattwire so the issue here is fundamentally - how do we switch to an Order->payment flow for membership renewals when some of the form submitters intent is not stored beyond the submission of the form - ie the form permit setting 'renewal date' and 'number of terms' but neither are stored beyond the form submission so if payment follows later there is no way to know the submitter's intent.

To my mind this is the blocker on switching to that preferred flow

Wow, lots of interesting thoughts on a two-day old conversation. @jptillman, thanks for your great contributions as a newer member, they are most welcome. I particularly liked the objective of an event oriented design.

A few responses here and there:

I too am very uneasy about trying to introduce a new pending renewal membership status. It feels like it will lead to grief similar to our overloading of contribution status.
@andrewhunt On backoffice contact summary form, an info warning comes up when trying to create a second membership of the same type for the same contact, but the second membership is created rather than the first one being extended, at least by default. Wasn't how I did it long ago with dgg, but heh, it's been merged.
@eileen When trying to create a checkbox price field for memberships, trying to add two for the same membership type leads to 'You have selected multiple memberships for the same organization or entity. Please review your selections and choose only one membership per entity.' I would support better validation across the fields in a priceset to prevent different fields from including the same membership type; or at a minimum, validating at time of purchase (both via API and form) that only one membership per type is being purchased.
I tend to think that membership_payment record would be a more appropriate place to put fields relating only to memberships, rather than on line items which might be donations or event registrations. At least it is more normalized. I don't personally like having required data for operations only appear in a log table. Looking at the line_item table, it doesn't seem like the number fields is that big of a concern yet as we're no where near the limit. But it does tend to get a huge number of records, and adding unnormalized fields would be a small performance hit on the very largest CiviCRM implementations like WMF. And there is a (bad) precedent for putting in https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Price/LineItem.xml#L119 which only relates to event registration line items. Slightly different consideration: when calling the order api (which doesn't correspond to an order table) we would want the arguments specifying the intent to override the membership date to be passed as line item parameters. So I would prefer to add fields to override the membership start date and membership end date, that would only be applied to a membership when a payment completes payment for the amount owing for the membership line item, on: a) membership payment, b) less preferred, but still good, line item. I haven't seen so far at least a good enough justification to create a whole new table.

I'm OK with adding line item metadata to line_item table. I hope to drop membership payment table one day....

Cool. I'm good with that.

changed the description

Hmm - there is one more type of metadata it would be nice to store & re-use when confirming - receipt text. ie the text the form-filler enters

I guess if there were multiple line items we could just store it on 'pick on, any one' and concatenate it. In practice it's only relevant to the renewal form at the moment - so the multiple line item wouldn't come up.

Also - would we empty out the field once the renewal is complete? (I don't think we need to answer now but if the concept grows to other types of line items it might avoid bloat)

MySQL doesn't use the whole maximum possible length of a varchar field for each row, but just one or two per row plus the the length of the string being stored. https://dev.mysql.com/doc/refman/8.0/en/storage-requirements.html#data-types-storage-reqs-strings

So for a rare long string I think we're good.

So what is the final list of fields? I would prefer separate ones rather than serialized data combining several php variables.

Any single long string will mean All rows in the table use that length

Fields = membership_num_terms, start_date, end_date, join_date (I dithered about the last but I think it could make sense) + maybe receipt_text

possibly status_id would be wanted

Actually, no, the storage of a VARCHAR(255) is not rows x space for max length of string. For each row, one calculates, with L length of string and M the maximum size of the field and L is the length of a particular's row's string: Sum of( L + 1 bytes if column values require 0 − 255 bytes, L + 2 bytes if values may require more than 255 bytes)

sorry - you are right - it's table width that can be affected - https://medium.com/galvanize/the-maximum-width-of-mysql-database-tables-76aa181b1441 - probably not an issue on this field

changed title from Proposal - new reserved membership status Pending Renewal to Proposal - store metadata on membership renewal on line item

changed the description

I've partially updated the summary - but I thought about an edge case - when the membership has been renewed in the meantime and THEN payment is completed - it's a bit edge case. I wonder about clearing it out if any date update takes place or storing the starting dates & only using that data if they are still true

My own take, without going into the fractal edges of the edge case, is that a relatively common scenario is a double payment on a membership. Perhaps one renewal is in process and a second happens separately before first finishes.

I think some will want to a refund, others will want to extend the membership for an extra term. If we extend term then people can manually correct membership term at time of manual refund. It's getting murky to figure out appropriate business actions. I wish we had a pattern for raising an incident with a human to review. On one or two custom projects we've sent enail to specific address but we've never had this pattern in core. Nor do we insist that a specific email be properly monitored. Though core does have an email defined for the org, I see it as more of a reception email than a financial admin one.

I want to pick this up again because we sort of converged on a 'solution' that landed in the too hard basket with a very resounding thud and has left the original bug, another bug and a whole lot of follow up work to make the order process work in the too hard basket with it.

The original proposal was in this PR https://github.com/civicrm/civicrm-core/pull/18582 & this variant https://github.com/civicrm/civicrm-core/pull/18618 and used the qty field on the default ('non price set') price set to store the number of terms that had been purchased.

The qty field is not currently editable for any membership type price fields - but it IS enterable (as membership no terms) on the form WHEN you are using the default ('non price set') price set. So there is no other currently available use case that could be affected.

The original concern was that a theoretical use case might be blocked off https://github.com/civicrm/civicrm-core/pull/18618#issuecomment-703302539

We could hard code qty to ONLY work for the default-non-price-set-price-set to limit any future constraints (if people think it's an issue - I would be a soft minus one on that). That price set is already voodoo so one could make a case that a bit more special witchcraft on it is fine

The proposal that otherwise came out of discussion on this ticket is to add more metadata to the line item - however, we would not be able to rely on that metadata without fixing a number of scenarios to save it correctly - and since we would be expecting new fields to be saved reliably we can't just 'leave the bits that already work working for now'. We start to get into a loop of changes we wouldn't want to make because of the state of the code - but there is no path to clean it up without a solution that facilitates the move to the Order api / preferred code.

By contrast we already have data in the qty field that is 'working-ish' and if we handle qty being > 1 (which is an easy change) then we can start working at fixing the 1 form that supports entering it as anything else and the Order api

The proposal that otherwise came out of discussion on this ticket is to add more metadata to the line item - however, we would not be able to rely on that metadata without fixing a number of scenarios to save it correctly.

My worry here is that if we don't do it properly we just end up in another mess. The mixture of pricesets/non-pricesets/is it a membership priceset/is it a event priceset etc. and other pricing stuff just makes it really hard to work with. With lineitems it becomes a single, clear point of reference and allows for any combination to be well described and queryable. I really think we need to be adding metadata there and not hacking it in elsewhere.

The priceset is a bit like the menu here whereas the lineitems should be describing exactly what was ordered. So you might have a priceset for a membership with a qty of 2 but that should be translated into something appropriate at the lineitem level and not looked at again once the form has been submitted.

A simple example was when we added order_reference to civicrm_financial_trxn. It was added to the schema long before it could be read/written to and we later improved the APIs to work with it. It is still not exposed in the UI and we still have a bit of a mess around the "legacy" trxn_id on the contribution but it's been done the right way.

So I'm not actually on board with the idea that adding membership_num_terms to the line item table is more 'proper' than treating qty as quantity - I had always assumed that in fact qty Did mean the quantity of what the line item is configured as that you are buying and all else aside it's more logical to me. I originally accepted that argument based on trying to accomodate it rather than agreeing with it.

I think this is fundamentally different to the order_reference field in that that field is really just used by an extension but we added it to core as it made sense data-model wise.

The handling of memberships is a fundamental feature and we have many complicated flows that interact with it. If we add a new membership_line_items field then the underlying assumption has to be 'this is only expected to be accurate if it is greater than 1' - if it is greater than 1 then it is that or because it has been fixed to be that. If it is not then it is either genuinely 1 OR it should be greater than 1 but we have not fixed the flow such that it is so it will default to 1. I guess you could argue that is the same as the qty field - but it would mean we are adding a new field under the understanding that it is both a key part of many flows and it is not necessarily accurate. That feels worse to me than treating this as bug fixes to make the existing field correct over the next 4-6 months.

I have only focussed on the alternative of adding the membership_line_items as a field as I think going fully for adding a metadata field & using that feels like it can be considered after other cleanup is done to get the code to a sane state (but that cleanup itself depends on us having a way forwards on multiple-terms-pending)

OK - I've thought about this some more and I think that on one hand I am struggling to get past the fact that the ONLY way entering a qty against a line item makes sense to me is 'I want to buy 2 of those' and if 'those' is a membership then buying 2 means 2 terms or 2 * x terms. So fundamentally I'm uncomfortable with NOT using qty in the way that seems logical.

However, if we accept that qty is not a usable field for the purpose of defining how many we have bought then we to use something else and it needs to meet the following needs

be optional / fall back to existing behaviour if not set - so for most membership purchases for the medium term metadata will be equal to []
not open up a range of expectations beyond the specific use case we are addressing. ie. we currently delete & rebuild line items and we don't want someone to say 'hey I've started storing custom data in that field & you lost it' or 'hey - you used to do an array merge on the data in that field so I started putting x in it and now you have refactored your code and my use case doesn't work'

I guess as long as we are deliberate about what we allow to be saved into it then metadata works - ie we can initially save an array holding just the membership_num_terms key if available in the LineItem.create and drop any other keys that might be passed in in the metadata array. Then we can edit the getMembershipNumTerms function to retrieve that if set.

Any expansion of this to facilitate anything else would then be supported by the schema but not by the code without concept-approval / addition of unit test cover over the Order->payment flow

I had always assumed that in fact qty Did mean the quantity of what the line item is configured as that you are buying and all else aside it's more logical to me.

I was thinking the same thing.

In my case, I've got a client that has an accountant and backend staff that need to see a history of the payments and the membership terms that these payments apply to. This can't be ephemeral data. In my searching I came across getNumTermsByContributionAndMembershipType() so I had assumed I finally had something to work with (after I fixed their price sets so that the multi-term price options were actually set to more than 1 term). But then in my testing I realized that back-end staff could subvert this by renewing a membership and set "Extend membership by [] membership periods". And the system will have no record that this was actually a payment for more than 1 term. (This is also when I discovered these secret price sets where membership_num_terms is always set to 1 and its not configurable.)

The most logical approach in my mind is to set the qty. If the price field option is a Membership of 1 and only 1 term (and there's no way to configure this or use a different price field), then if the staff are applying this multiple times then I can only think that the qty needs to increase.

To me this is not "metadata" but integral to how line items should work. I also hope, for the sake of reporting, that the membership number of terms doesn't get stuffed into a JSON or serialized field.

I also hope, for the sake of reporting, that the membership number of terms doesn't get stuffed into a JSON or serialized field.

Good point, @herbdool. A serialized field for metadata doesn't prevent us from tying qty to the number of membership terms being purchased. In my own systems it would also be preferred to do it that way for reporting purposes. My point previously is that it would move the decision of what the available information means to the membership component where it belongs, and keep membership-specific concepts from leaking into the financial management.

My thinking is that it should be up to the component for whom the transaction data/metadata is intended to determine what it represents. So the membership component would get notified that a transaction occurred which had metadata intended for it and it would receive the transaction data and metadata and do what it deems appropriate with it, such as interpreting qty as the number of memberships purchased.

The really nice thing about this approach is that, without modifying the financial handling code at all, the membership component could allow someone to optionally modify this behavior through settings or custom code.

Considering this has been in discussion for 7 months, and the experienced folks don't seem too keen on my suggestion for what appear to be good reasons, I'm guessing that my points here may be moot.

I've tested the concept as set out in this PR: https://github.com/civicrm/civicrm-core/pull/18618 and it provides the results I was hoping for:

... at least in terms of being able to create a report on which line items correspond to what membership terms/periods. I haven't tested the "pay-later renewal" part so I can't speak to how that is best handled.

@jptillman I think your suggestions are valued here - I'm hearing a variety of responses from the experienced folks too so I think this is still up in the air. I'm just wrapping my head around this all and going back to read people's objections to understand them better.

Some comments seem to suggest that we don't want to blur qty with membership terms. I don't see how it gets blurred. A price field value can have 1 or more member terms if it's a membership type of price field. The line item doesn't care about that, it just records the quantity of that option. It's up to the membership reporting or membership renewal process to interpret that.

I don't understand the objection raised here: https://github.com/civicrm/civicrm-core/pull/18618#issuecomment-703302539 by @andrewhunt :

A common use case is to have a single membership type and offer discounts for buying multiple terms at once. A price set might have a radio field with $30 for one year and $50 for two years, for example. Theoretically, someone might get qty of 3, membership_num_terms of 2 because they want to pay $150 to get 6 years of membership.

This seems fine (if possible in the future). Isn't it the same equation: qty * membership_num_terms? Unless I'm missing something I don't see how the PR https://github.com/civicrm/civicrm-core/pull/18618 would prevent that.

And above @mattwire:

The priceset is a bit like the menu here whereas the lineitems should be describing exactly what was ordered. So you might have a priceset for a membership with a qty of 2 but that should be translated into something appropriate at the lineitem level and not looked at again once the form has been submitted.

This describes the ideal case, it seems to me, and not how the system currently works, correct? Right now to determine the number of terms (in getNumTermsByContributionAndMembershipType() and in reports) it needs to refer back to the price set field value and ignore the quantity. Changing this to account for quantity won't prevent the ideal of "translated into something appropriate at the lineitem level".

I think @mattwire's comment about "translated into something appropriate at the lineitem level" may have been relevant to something that has been on my mind about this, which is the purchase of some sort of "package" which includes multiple membership terms. If you identify the line item as "Super Best Deal Package" and it includes an event registration and 2 membership terms, then you can't easily match qty to the membership terms. But that's kind of on the implementer to deal with it.

I would advocate doing both: Keep the membership_num_terms in the serialized metadata and just use qty for cost calculation. Doing this, we allow the implementer to make the product/membership_term ratio be 1/1 if they wish, or they can break that correlation. That would allow folks like you and me for whom the 1/1 ratio matters to maintain control (with the implicit understanding that in our systems it's 1 qty = 1 membership term), but also allow those who want to get fancy to do their thing.

As long as we maintain the 1/1 ratio in our own systems, you and I have the reporting numbers we want, and the flexibility (and integrity) of the system is maintained for other uses.

@jptillman "If you identify the line item as "Super Best Deal Package" and it includes an event registration and 2 membership terms, then you can't easily match qty to the membership terms." - the schema doesn't really support this - each line item is attached to a maximum of one membership or one participant (although participants can have 'sub-participant' records in some historical ugliness)

So I think your super deal package is a combination of line items rather than just one

So I think your super deal package is a combination of line items rather than just one

I see, so not as much of a concern, then. There's still the issue of what to do with special price sets, like the attached, where I created discounted prices for multiple membership terms. The line item shows a qty of 1 even though I purchased 3 terms. So that is still sort of a "package deal" like I was discussing. A qty of 3 would require the item price to be $6.67 and would never total out properly. As it stands, I've bought "1" of "something".

Here's the line item display for the order (I selected the mug before paying)

@jptillman - your point about the rounding is a good one & it makes a difference to how I see this - so I think I can now see the case for adding membership_num_terms to the line_item table - and I guess that would be calculated total number of terms for the line item - ie you could go 2 ways

total_number_of_terms = qty * membership_num_terms or total_number_of_terms = membership_num_terms

The former feels more correct/ flexible - the latter maybe is more queryable. In practice we are talking about a type of option value which always has a qty of 1 anyway

I've updated the issue summary to relfect ^^

changed the description

Apologies, if I'm bringing up something which has been covered elsewhere. I'm wondering why CiviCRM doesn't have a civicrm_order table like in Drupal Commerce? Or is some other table filling that role more or less (civicrm_contribution)? I was thinking that civicrm_line_item are individual items in an order so would adding payment_completion_metadata to that table mean there would be duplicate information on each of the line items that are part of the same contribution?

In Drupal Commerce each line item has an order_id (plus quantity, created, changed, and a serialized data blob). And commerce_order has status (cart, checkout_review, etc) and a serialized data blob, among other fields. Maybe there's a role for a civicrm_order table?

Looking at civicrm_contribution it seems like it combines concepts from both commerce_order and commerce_payment_transactions. Is part of that a holdover from before civicrm_financial_trxn was introduced?

@herbdool yeah forever ago we basically decided the contribution is effectively the order table - with financial_trxn being payments & line items being line items. As you say, there are holdovers from before that time

On the payment_completion_metadata - yeah I think you are right - if we put membership_num_terms on the line item level then the other 2 things that I feel confident we need (a way to save the selected renewal date & text for the email so the IPN can access them when the form is no more) probably DO belong at the contribution level. I originally considered a broader scope that would potentially address other issues but I think all of those things are out of scope now - I'll update the summary to say it should go on the contribution table

changed the description

mentioned in issue core#2634 (closed)

Just noting that I've realised adding membership_num_terms to the line_item table (which I think is agreed here) will unblock the v4 membership api progress #29 (moved)

Note that this field will be NULL in many cases initially due to existing flows not setting it & null being preferable IMHO to populating it with what we 'think' it should be. NULL will be assumed to be 1

I've put up a PR https://github.com/civicrm/civicrm-core/pull/20672 with the change just to add membership_num_terms to civicrm_line_item

This discussion is pretty hard to follow by now - I've summarised on the PR

mentioned in commit core@cf7384c1

mentioned in commit core@40ed7906

closed

moved to core#3311 (closed)

Proposal - store metadata on membership renewal on line item

Child items ...

Activity