Development issueshttps://lab.civicrm.org/groups/dev/-/issues2023-09-25T14:58:11Zhttps://lab.civicrm.org/dev/core/-/issues/1516Discussion ticket for handling multivalued tokens in pdfs/emails2023-09-25T14:58:11ZDaveDDiscussion ticket for handling multivalued tokens in pdfs/emailsIn particular related to:
* https://github.com/civicrm/civicrm-core/pull/12012
* and its rebased version https://github.com/civicrm/civicrm-core/pull/16200
* https://github.com/civicrm/civicrm-core/pull/16208
There are a number of way...In particular related to:
* https://github.com/civicrm/civicrm-core/pull/12012
* and its rebased version https://github.com/civicrm/civicrm-core/pull/16200
* https://github.com/civicrm/civicrm-core/pull/16208
There are a number of ways to deal with multivalued fields when you need to produce scalar output based on the field.
For example:
* Take the "first" one.
* This has the disadvantage that "first" doesn't really mean anything, especially if the order is not guaranteed to be the same each time.
* Take the "last" one.
* Ditto.
* Combine them separated by commas.
* This has the disadvantage that it may not make any sense or might look silly. Consider something like the "With Contact" on an activity. Suppose there are 3. Suppose you want to include the primary phone of the contact. Your pdf might look like this. It's not terrible, but now think about adding in address too.
* With Contact: John Smith, Mary Brown, Indiana Jones
* Phone: 555-123-2222, , 555-383-8383
* And it's not clear to me that it can guarantee that the phone values are in the same order as the names when the tokens are separate.
* Do as in https://github.com/civicrm/civicrm-core/pull/12012, where you give the template designer the option to specify a numeric suffix on the token, e.g. {activity.target_2_display_name}. The way it's implemented there has some limitations:
* The order is still arbitrary - what does "2" actually mean?
* The template designer doesn't know max(N), so they can't easily do something like concatenate all of them if they want to.
* Provide some shorthand tokens for some common uses, e.g.
* {activity.target_1_phone} means arbitrarily take the first one.
* {activity.target_COMBINE_phone} means combine all with commas.
* {activity.target_ALL_phone} means give an array and I'll do some smarty-based looping in my template to get what I want.
Further, there are some situations where you might not want some recipients to see some of the other values. An example might be activity.case_id as in [16208](https://github.com/civicrm/civicrm-core/pull/16208). There maybe it makes sense to have a token that means "concatenate all the cases where the recipient has a role, and leave out the others". A similar one might apply for activity.target_display_name, where it might mean "concatenate all the with contact display names for activities where the recipient is one of the activity_contacts.
A related note that's just food for thought: In my other life, I always try to set up 2 fields when somebody asks for something multivalued. For example "services performed". Somebody might come to you asking for a couple of the different types of service you offer. You can designate one as the primary service, which is a single-valued field, which then makes reporting work because you don't run into double-counting, and then have a secondary services field that is multi-valued, which you don't use for reporting, and in emails/pdfs you can concatenate it without it being arbitrary because you would also output the primary which is single-valued. That may not be useful right now, but this is a discussion ticket.https://lab.civicrm.org/dev/drupal/-/issues/101Error cancelling Drupal user2020-01-06T21:44:57ZedvanleeuwenError cancelling Drupal userWhen I cancel a Drupal user account, I get a non-descriptive error and cancellation has not been done. Doing this a second time does cancel the user, but the user relation is still visible in Civi because the entry in civicrm_uf_match is...When I cancel a Drupal user account, I get a non-descriptive error and cancellation has not been done. Doing this a second time does cancel the user, but the user relation is still visible in Civi because the entry in civicrm_uf_match is not removed.
Using Drupal 7.67 and CiviCRM 5.20.2.https://lab.civicrm.org/dev/release/-/issues/13Please consider using CVE identifies for security issues2020-01-03T14:13:55ZDmitry SmirnovPlease consider using CVE identifies for security issuesInstead of vendor identifiers (e.g. `CIVI-SA-2019-24`) please consider using standard CVE identifies for security issues.
Here is some reasoning from the Debian Security Team:
> The good thing on having a CVE id for the vulnerabilities...Instead of vendor identifiers (e.g. `CIVI-SA-2019-24`) please consider using standard CVE identifies for security issues.
Here is some reasoning from the Debian Security Team:
> The good thing on having a CVE id for the vulnerabilities is helping
> other vendors to track the issues properly 'cross-vendor' in an unique
> way. If every upstream would use individual identifiers to track their
> vulnerabilities, this makes the work of downsteams security teams much
> harder. Nowdays MITRE has improved a lot on their processes on
> assigning CVEs, and good filled reports at https://cveform.mitre.org/
> get fastly assigned a CVE respectively (this somehow depends though on
> how good the report is done). I know some upstreams did in past make
> frustrating experiations, and do not want to try that out again.
See also [Debian Upstream Guide](https://wiki.debian.org/UpstreamGuide).
Thank you.https://lab.civicrm.org/dev/release/-/issues/125.20: open source compliance: non-free 'vendor/togos/gitignore'2020-01-07T08:34:12ZDmitry Smirnov5.20: open source compliance: non-free 'vendor/togos/gitignore'Release management should exercise more care in regards to Open Source compliance of the sub-vendored re-distributed components.
For instance, in release 5.20 `vendor/togos/gitignore` is a non-free component due to absence of licensing ...Release management should exercise more care in regards to Open Source compliance of the sub-vendored re-distributed components.
For instance, in release 5.20 `vendor/togos/gitignore` is a non-free component due to absence of licensing and copyright information.
https://github.com/TOGoS/PHPGitIgnore/issues/1https://lab.civicrm.org/dev/core/-/issues/1486Deadlocks on acl_cache2023-09-12T18:50:06ZeileenDeadlocks on acl_cacheOne of the prevalent deadlocks we see is on the acl_contact_cache. I've found that I can replicate this locally fairly consistently just by running 2 sets of tests at once.
The thing that is somewhat odd about this is that we don't use ...One of the prevalent deadlocks we see is on the acl_contact_cache. I've found that I can replicate this locally fairly consistently just by running 2 sets of tests at once.
The thing that is somewhat odd about this is that we don't use ACLs at all so in fact the table is always empty. Notably the table is also extremely slow to truncate - despite being empty.
**What happens**
The flow we have is
1) api call to create a contact
2) when the contact is created CRM_Contact_BAO_Contact_Utils::clearContactCaches() is called. It hits a deadlock because
a) it has an FK to contact.id (& in our case it has an FK and an index seemingly)
b) it is called a lot
3) it retries and succeeds
4) we then try to add an email - the add function fails on a foreign key constraint because the earlier deadlock rolled the contact create back
**Proposals**
I think there are a number of things we can do & we should do some combo:
1) Make the delete function less expensive & less locky
a) Add an index on modified date - we are constantly deleting where modified_date < x so let's index it
b) remove the index on acl_id - we ALSO have an FK so having both is probably harmless-but-confusing
c) alter the FK on contact ID to be an index not a foreign key. This is perhaps a confusing suggestion but it is also the one that will make a real dent in these deadlocks. Remember this is a cache table not a 'persistent' table. The impact of the rows outliving the existence of the contact for a little bit is basically none because we don't do ACL lookups on non-existent contacts (this is also potentially helpful for better handling 0 or anonymous contact_id where we've had to work around this before
2) Protect against conflict through mysql locks. I think we can use a mysql lock to avoid 2 processes attempting this flush at once. I'll do a PR for this
3) How often do we REALLY need to flush those caches. Maybe we could cache a 'last flushed' key & flush no more than every 30 seconds.
4) Consider checking if the table is empty before attempting to empty it.
5) Review the places where acl cache is called from & decide how necessary there are - here is the list:
**CRM_ACL_BAO_Cache::resetCache**
CRM/Contact/BAO/Contact/Utils.php: CRM_ACL_BAO_Cache::resetCache();
CRM/Core/BAO/Cache.php: CRM_ACL_BAO_Cache::resetCache();
CRM/Utils/System.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/Page/EntityRole.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/Form/ACLBasic.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/Form/EntityRole.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/BAO/ACL.php: CRM_ACL_BAO_Cache::resetCache();
**CRM_Contact_BAO_Contact_Utils::clearContactCaches()**
CRM/Contact/BAO/Contact/Utils.php: public static function clearContactCaches($isEmptyPrevNextTable = FALSE) {
CRM/Contact/BAO/GroupContact.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
CRM/Contact/BAO/GroupContact.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
CRM/Contact/BAO/Contact.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
CRM/Contact/Import/Form/Preview.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches(TRUE);
bin/cli.class.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
6) A bigger task perhaps - but permit opportunistic flushing to be turned off in favour of a cron (per group contact cache)
**Deadlock output**
LATEST DETECTED DEADLOCK
------------------------
2019-12-19 11:29:45 0x700010cd2000
*** (1) TRANSACTION:
TRANSACTION 34518715, ACTIVE 1 sec starting index read
mysql tables in use 1, locked 1
LOCK WAIT 11 lock struct(s), heap size 1136, 4 row lock(s), undo log entries 7
MySQL thread id 571, OS thread handle 123145583353856, query id 709677 localhost 127.0.0.1 drupalcivi_lkznr updating
DELETE
FROM civicrm_acl_cache
WHERE modified_date IS NULL
OR (modified_date <= '20191218222444')
*** (1) WAITING FOR THIS LOCK TO BE GRANTED:
RECORD LOCKS space id 143182 page no 3 n bits 80 index PRIMARY of table `drupalcivi_lkznr`.`civicrm_acl_cache` trx id 34518715 lock_mode X waiting
Record lock, heap no 2 PHYSICAL RECORD: n_fields 6; compact format; info bits 32
0: len 4; hex 00003c14; asc < ;;
1: len 6; hex 0000020eb6ba; asc ;;
2: len 7; hex 3b000001442ac5; asc ; D* ;;
3: SQL NULL;
4: len 4; hex 00000002; asc ;;
5: SQL NULL;
*** (2) TRANSACTION:
TRANSACTION 34518714, ACTIVE 1 sec starting index read
mysql tables in use 2, locked 2
66 lock struct(s), heap size 8400, 71 row lock(s), undo log entries 74
MySQL thread id 573, OS thread handle 123145584189440, query id 710022 localhost 127.0.0.1 drupalcivi_lkznr updating
UPDATE civicrm_contact SET contact_type = 'Organization' , contact_sub_type = NULL , email_greeting_id = NULL , postal_greeting_id = NULL , addressee_id = 3 , employer_id = 15055 WHERE ( civicrm_contact.id = 15055 )
*** (2) HOLDS THE LOCK(S):
RECORD LOCKS space id 143182 page no 3 n bits 80 index PRIMARY of table `drupalcivi_lkznr`.`civicrm_acl_cache` trx id 34518714 lock_mode X
Record lock, heap no 1 PHYSICAL RECORD: n_fields 1; compact format; info bits 0
0: len 8; hex 73757072656d756d; asc supremum;;seamusleeseamusleehttps://lab.civicrm.org/dev/core/-/issues/1481Revisiting deadlocks2022-09-01T10:27:33ZeileenRevisiting deadlocksSome time ago we added handling to the DataObject class to catch and retry deadlocks. This seems to work and be helpful in some cases but in others it doesn't work and is possibly harmful. I want to re-open the discussion on how & where ...Some time ago we added handling to the DataObject class to catch and retry deadlocks. This seems to work and be helpful in some cases but in others it doesn't work and is possibly harmful. I want to re-open the discussion on how & where we catch them & roll them back.
**Why do we have deadlocks**
Deadlocks are a 'natural' part of mysql scalability. Mysql tries to manage contention under load to the extent that can be done at the DB layer, allowing tables & rows to be locked & queuing transactions that need to use those resources to complete after that. However, in some cases mysql cannot resolve it and it returns a deadlock error. The expectation is that the application layer will handle & retry.
For example the query in this PR was causing deadlocks - https://github.com/civicrm/civicrm-core/pull/16080 - here is how I think the flow worked - note the queries are not slow but this is under high volume.
1) contact create is called by 3 different processes in pretty quick succession
2) The query to update the employer name field is called by all 3
3) the first query 'gets the lock' - the other 2 get shared locks while they wait
4) the first query finishes. Neither of the other 2 can get the exclusive lock as they both have shared locks
5) one is reverted & a deadlock is thrown
6) the deadlock is caught in packages/DB/Dataobject
7) the Dataobject code retries the query and succeeds. The transaction succeeds.
**When don't we do a good job with deadlocks**
The above scenario is good because once the deadlock was resolved we were able to retry and it worked. However, it turns out that mysql often rolls back more than just the query it was doing when it decided there was a deadlock. Per https://dev.mysql.com/doc/refman/8.0/en/innodb-deadlock-detection.html " InnoDB automatically detects transaction deadlocks and rolls back a transaction or transactions to break the deadlock."
We are seeing a consistent pattern where under high volume we see deadlocks on ```INSERT INTO civicrm_email``` - via contact.create api (called in turn from a job api by drush). The INSERT email fails but on retry it hits a constraint error - because unknown to the php layer the ```INSERT INTO civicrm_contact ``` statement was also rolled back.
In this case the DB state is hopefully still consistent as the failure should have triggered more rollbacks but it's at least theoretically possible to have lost data in the rollback & then have the query succeed when retried - so the database is in an inconsistent state.
My high level conclusion is that we should be catching deadlocks & retrying higher up the stack - but I'm still trying to figure out where.https://lab.civicrm.org/dev/financial/-/issues/112Add FormRule to force user to remove "&" from both "Billing First Name" and "...2019-12-19T08:39:04ZKarinGAdd FormRule to force user to remove "&" from both "Billing First Name" and "Billing Last Name" fields**Issue:**
First Name field can be filled out as e.g. "Jon & Mary";
That "&" can cause issues with Payment Processors when it's passed on as part of the Cardholder Name - resulting in rejecting properly entered Credit Card information....**Issue:**
First Name field can be filled out as e.g. "Jon & Mary";
That "&" can cause issues with Payment Processors when it's passed on as part of the Cardholder Name - resulting in rejecting properly entered Credit Card information.
Credit Card Payments with iATS Payments will get a REJ:23 failure code when the Cardholder name contains a "&" - I did some research to see if this is iATS Payments' specific, but I don't think it is.
I found e.g. CartPay -> also has error codes ERROR: 54043: "The Billing Firstname contains invalid characters."
I also found SagePay has such error codes - for every single text field actually:
```
"5037 : The Delivery Address contains invalid characters.","5037 : The Delivery Address contains invalid characters."
"5038 : The Delivery Phone contains invalid characters.","5038 : The Delivery Phone contains invalid characters."
"5039 : The Delivery City contains invalid characters.","5039 : The Delivery City contains invalid characters."
"5040 : The Billing Surname contains invalid characters.","5040 : The Billing Surname contains invalid characters."
"5041 : The Billing Firstname contains invalid characters.","5041 : The Billing Firstname contains invalid characters."
"5042 : The Billing Address1 contains invalid characters.","5042 : The Billing Address1 contains invalid characters."
"5043 : The Billing Address2 contains invalid characters.","5043 : The Billing Address2 contains invalid characters."
"5044 : The Billing City contains invalid characters.","5044 : The Billing City contains invalid characters."
"5045 : The Billing Phone contains invalid characters.","5045 : The Billing Phone contains invalid characters."
"5046 : The Delivery Surname contains invalid characters.","5046 : The Delivery Surname contains invalid characters."
"5047 : The Delivery Firstname contains invalid characters.","5047 : The Delivery Firstname contains invalid characters."
"5048 : The Delivery Address1 contains invalid characters.","5048 : The Delivery Address1 contains invalid characters."
"5049 : The Delivery Address2 contains invalid characters.","5049 : The Delivery Address2 contains invalid characters."
"5050 : The Billing Address contains invalid characters.","5050 : The Billing Address contains invalid characters."
"5051 : The Contact Number contains invalid characters.","5051 : The Contact Number contains invalid characters."
"5052 : The Customer Name contains invalid characters.","5052 : The Customer Name contains invalid characters."
"5053 : The Email Message contains invalid characters.","5053 : The Email Message contains invalid characters."
"5054 : The Cardholder Name contains invalid characters.","5054 : The Cardholder Name contains invalid characters."
"5055 : A Postcode field contains invalid characters.","5055 : A Postcode field contains invalid characters."
"5056 : The Contact Fax contains invalid characters.","5056 : The Contact Fax contains invalid characters."
```
This Image illustrates -> issue (on the left) -> no issue (on the right). Of course the checkbox for My billing address is the same as above is what copies the First Name and Last Name fields down into the Cardholder name bits. The form rule can look for "&" and then tell the user to remove it. Note the user can still leave "Mark & Karin" in the Name and Address Profile. But we would prevent it from going into the Billing details.
![image](/uploads/bd1131f6790be6d085fe3962d4017ffa/image.png)https://lab.civicrm.org/dev/financial/-/issues/110Discussion: Should you be able to edit a cancelled or completed Contribution ...2019-12-19T08:38:28ZseamusleeDiscussion: Should you be able to edit a cancelled or completed Contribution RecurAt present if someone cancels the contribution recur object or is cancelled for them (in the chris chilla eway extension this happens if the payment fails and the card file details that are retreived indicate the card has expired) you ca...At present if someone cancels the contribution recur object or is cancelled for them (in the chris chilla eway extension this happens if the payment fails and the card file details that are retreived indicate the card has expired) you cannot edit them as they are in the inactive section, I'm curious if people think its correct that those recur objects can't be edited say to re-activeate them etchttps://lab.civicrm.org/dev/drupal/-/issues/97Status report update for Drupal (similar to what was done for Backdrop)2020-01-29T14:53:02ZlarynStatus report update for Drupal (similar to what was done for Backdrop)I wrote a PR for Backdrop some time back and have been meaning to check if it would be desired for Drupal 7 as well -- if so I would be happy to base a MR here off of what I did there. Here's a visual of how it looks in Backdrop's Status...I wrote a PR for Backdrop some time back and have been meaning to check if it would be desired for Drupal 7 as well -- if so I would be happy to base a MR here off of what I did there. Here's a visual of how it looks in Backdrop's Status Report page (Drupal 7 status report would obviously look a little different):
![Screen_Shot_2019-12-04_at_7.10.28_PM](/uploads/0a83d103f2aec9652797d3ce71c41345/Screen_Shot_2019-12-04_at_7.10.28_PM.jpg)
Would a version of this for Drupal 7 be accepted?https://lab.civicrm.org/dev/core/-/issues/1437Creating relationship with start date in future incorrectly shows relationshi...2023-01-17T01:36:45Zellen_compucorpCreating relationship with start date in future incorrectly shows relationship as activeOverview
----------------------------------------
Creating relationship with start date in future incorrectly shows relationship as active
Reproduction steps
----------------------------------------
1. Go to a contact record, then go to...Overview
----------------------------------------
Creating relationship with start date in future incorrectly shows relationship as active
Reproduction steps
----------------------------------------
1. Go to a contact record, then go to the relationships tab
1. Create a new relationship of any type
1. Give the relationship a start date that is in the future
1. Save the new relationship
Current behaviour
----------------------------------------
The relationship is saved correctly, but displays within 'current relationships' in the top half of the relationships tab.
![6AD41E8B-5DD7-4497-84A5-96600B621E9A](/uploads/a43484418ac5b913033190c9e3a52f73/6AD41E8B-5DD7-4497-84A5-96600B621E9A.jpg)
Expected behaviour
----------------------------------------
As the relationship has not yet started, the relationship should show as inactive up until the start date; at which point it should show as current.
Environment information
----------------------------------------
* __Browser:__ Chrome
* __CiviCRM:__ 5.19 (also tested on 5.15 on this test site and contact record: https://civicrm.demo.civihosting.com/civicrm/contact/view?reset=1&cid=98)
* __PHP:__ _7.0
* __CMS:__ Drupal
Comments
----------------------------------------
Contact record from my testing on a demo site: https://civicrm.demo.civihosting.com/civicrm/contact/view?reset=1&cid=98
cc @jamienovick1 @shitijghttps://lab.civicrm.org/dev/release/-/issues/10Security Releases - Update post-release communications workflow2019-11-27T23:11:00ZtottenSecurity Releases - Update post-release communications workflow__Synopsis__: When a new security release goes out, one may wish to notify as many users as possible. This requires communications across several different media. The aim of this issue is to define a smoother workflow for improved consis...__Synopsis__: When a new security release goes out, one may wish to notify as many users as possible. This requires communications across several different media. The aim of this issue is to define a smoother workflow for improved consistency.
__Target Media__:
The communications about a recent release may hit the following media:
* `chat.civicrm.org` (public groups) - "Town square", "dev", "product-maintenance"
* `chat.civicrm.org` (private groups) - "security", "ESR", "mergers"
* `latest.civicrm.org` - Versions listing (which feeds `civicrm.org/download` and in-app notices)
* `sourceforge.net` - Default download
* `civicrm.org` - Security advisories
* `civicrm.org` - Blog
* `civicrm.org` - "Security Notifications" mailing list
* `twitter.com` - `@civicrm` account
* `facebook.com` - CiviCRM group
__Considerations__:
* The broad communications for monthly-releases and security-releases are similar, but security-releases have the higher bar because they are more urgent, leading to more time-sensitivity and a longer list of media. Consequently, if we solve security-releases, then our monthly-releases can follow a simpler subset of the workflow.
* The list of media has tended to grow over time.
* The people who post to each medium has varied overtime.
* The knowledge/access of posting in different media are spread among different people.
* The best documents to refer people to are generally (a) the blog article and/or (b) the release-notes.
* The document [any-announce.md](https://lab.civicrm.org/dev/release/blob/master/doc/any-announce.md) proscribes some particular steps. It is slightly out-of-date, and includes a range of tasks which have (for at least... 5 years?) been split among different people.
* Today, the emotional experience of getting announcements is a bit like this: *You sign up for 3 channels, and then you don't see a notice on whichever one is your favorite, and someone tells you that you need to signup for more.*
* Today, the emotional experience of sending announcements is a bit like this: *You post a message to 5 channels, and then someone says that you didn't post to a 6th channel that you don't personally use.*
* It is conceivable to use a smaller list of feeds (like `latest.civicrm.org` and `release-notes`) to populate more channels automatically. This would require case-by-case technical implementation. Alternatively, for media where automation has poor cost/benefit, we can have a person responsible for each medium. This would likely create some lag (b/c it's hard to precisely coordinate timing across multiple continents). Any style of contribution (*automating a medium or standing sentry for a medium*) is useful.
__Proposal :one:__:
1. Team organization
1. There is a private MM group for people involved with release communications.
2. Rename the current `mergers` group to `releasers` or `release-team` or somesuch -- since this is a more accurate reflection of how the group works.
3. In the the GL project `dev/release`, add a wiki page with a list of media and the party responsible for each medium. Anyone added to this list must also be added to MM.
4. The MM channel has sticky link at the top to the wiki page.
2. For post-release communications, the workflow goes as follows:
1. Tarball and tag information goes up to `download.civicrm.org`, `sf.net`, `latest.civicrm.org`. The release is live.
2. The person who uploads then sends a notification to all `releasers` that the release is live.
3. Each person who signed up for release-comm's gets the ping and does thing their thing.
__Meta__
* If additional proposals are made, please give them an identifying code; and, if possible, include a summary in the description of this issue.
* Related: [Security Releases - Update pre-release communications workflow](https://lab.civicrm.org/dev/release/issues/11)https://lab.civicrm.org/dev/core/-/issues/1408The DAO "FreshnessTest" in the jenkins test suite does nothing because setup....2023-01-08T05:19:21ZDaveDThe DAO "FreshnessTest" in the jenkins test suite does nothing because setup.sh runs before itThe test itself works (CRM_Core_CodeGen_FreshnessTest::testDAOs()), but because in jenkins runs setup.sh gets run near the start of the build, a DAO will appear to be updated even if the git tree doesn't have the updated DAO.
The purpos...The test itself works (CRM_Core_CodeGen_FreshnessTest::testDAOs()), but because in jenkins runs setup.sh gets run near the start of the build, a DAO will appear to be updated even if the git tree doesn't have the updated DAO.
The purpose of the freshness test seems to be to make sure you don't forget to include the DAO in PRs when you make changes to an xml schema to add/change/remove a field in core tables. But it's not doing that currently in jenkins runs because it's always comparing against DAOs after setup.sh/GenCode.php has run.
I'm not sure if there's an easy fix, or maybe the test should be removed and replaced with some developer documentation? It wouldn't even work in a local test suite run if you've run GenCode.php, which you would have had to do to physically try out your schema changes.https://lab.civicrm.org/dev/financial/-/issues/108Disabling a price option or price field causes to be zeroed out and financial...2019-11-19T09:22:41ZseamusleeDisabling a price option or price field causes to be zeroed out and financial integrity issues when changing fee selectionsTo reproduce this error:
1. Create a Price set with > 1 price fields
2. Register and pay for someone using just 1 of those price fields
3. Now disable the price field that was used in step 2
4. Navigate to change fee selections to now s...To reproduce this error:
1. Create a Price set with > 1 price fields
2. Register and pay for someone using just 1 of those price fields
3. Now disable the price field that was used in step 2
4. Navigate to change fee selections to now select an option from the 2nd price field
5. Notice that the qty for the first price field gets zeroed out and no new financial trxn is created sensibly.
I believe @KarinG replicated this in chat https://chat.civicrm.org/civicrm/pl/ap5bif4gsifeuc57yftbxnmynh a work around is to rather than disabling the price option is to set the visibility to be admin only. The situation we were having is we were offering a catered option and an uncatered option for an event and wanted to stop people registering for the catered option at a point in time.https://lab.civicrm.org/dev/core/-/issues/1403Change "Contribution Received Date" to be "Contribution date" where is exists...2024-01-28T01:17:26ZJamie Novick - CompucoChange "Contribution Received Date" to be "Contribution date" where is exists and basically sort this Contribution dates mess out once and for allOk... sorry if this comes across as a bit of a rant but as a qualified accountant this has been driving me crazy for years and I think we've been skirting around this issue for sometime. I wanted to put in a gitlab to start a discussion ...Ok... sorry if this comes across as a bit of a rant but as a qualified accountant this has been driving me crazy for years and I think we've been skirting around this issue for sometime. I wanted to put in a gitlab to start a discussion to allow us to sort this out once and for all.
**Overview:**
----------------------------------------
CiviCRM has a core field on the contribution called "receive_date". This shows up on various screens and has led to significant debate about whether it should be a required field or not, and what it's real use case is. For example discussions below where debate has raged on how it should work:
https://lab.civicrm.org/dev/core/issues/1057
https://lab.civicrm.org/dev/core/issues/680
https://lab.civicrm.org/dev/core/issues/1140 - slightly different but related re: accruals accounting
https://lab.civicrm.org/dev/core/issues/1402
**The issue:**
----------------------------------------
So... there are a few problems with this field:
a) The idea of a "received date" is a bit meaningless (and confusing to users) when you have a pending contribution. You haven't quite received anything yet.
b) The idea of a "received date" conflicts with the payments dates if you have multiple payments (or could even be made to be different from the payment date) again which is nonsensical.
I assume contribution received date was a field that pre-dated the CiviCRM accounting implementation and hence perhaps hasn't kept up with the reality of the situation. In any case I believe this is causing some real issues now and should be sorted out.
Note the field is used:
1) As the date when initial accounting transactions are made. i.e. create a new pending contribution it will be the date of the Debit and Credits / transaction recorded in CiviCRM and hence by default it is the "revenue recognition date" for account purposes for the income.
This then leads to a multitude of other issues:
a) If I "complete" a pending membership contribution for a new membership by recording a payment, what should the start date of the membership be? (see: https://lab.civicrm.org/dev/core/issues/1402)
b) We've then introduced another field called "revenue_recognition_date", which should actually be field 1 above - as that is when we are recognising the transactions. We shouldn't have another field for this.
c) For some reason we now have a field on the invoice called "invoice date" which prints todays date which again is incorrect as it should be the same as item 1 above. The invoice should reflect the revenue recognition date. https://civicrm.stackexchange.com/questions/21128/how-do-i-print-the-received-date-on-an-invoice
**Proposed solution:**
----------------------------------------
The solution is relatively simple. We should change the "received_date" field to simply be the "Contribution date".
| Invoicing? | Contribution status | What the field means | Changes required to CiviCRM |
| ------ | ------ | ------ | ------ |
| Not using invoices | Pending | Represents the date the contribution was recorded on the system. | When later marking the contribution as completed give users the option to update the date or better still don't offer the option to complete a contribution and simply only allow users to enter the payment separately.
| Not using invoices | Completed | Represents the date the contribution was recorded on the system but also record a payment on the same date.
| Using invoices | Pending | Represents the date the invoice was recorded on the system and hence the invoice date and the revenue recognition date. | We should update the invoices to use this date as the invoice date. This field should not be updated when marking the invoice as complete (or better still don't offer the option to complete a contribution and simply only allow users to enter the payment separately.)
| Using invoices | Completed | Represents the date the invoice was recorded on the system but also the date the payment for that invoice was received. Ideally we would split the invoice and payment sections from each other on the "create new" form.
**Future:**
----------------------------------------
At some date in the future it would then be good to properly decouple payments from contributions and get rid of the terrible contribution status field once and for all.
Basically the contribution would reflect an order or invoice object, i.e. the obligation to pay for something. The payment transaction then would be separate and would represent the actual payment of that obligation. Accounting software has done this correctly for years so it's about time CiviCRM did too!
@mattwire @eileen @KarinG @dylan-circle @jaapjansma @JoeMurray @ErikHommel @fabian_SYSTOPIA and a few Compucorp names also - @omar_compucorp @Miya27
I'm sure you will all have opinions on this...https://lab.civicrm.org/dev/financial/-/issues/106on Order API test, pass parameters required to create line items, not line items2020-05-28T06:51:36ZJoeMurrayon Order API test, pass parameters required to create line items, not line itemsThe idea of the order api is to provide a wrapper around creating a contribution and all of the line item objects, eg the membership created when buying a membership, or the event registration when buying a ticket.
In the specification ...The idea of the order api is to provide a wrapper around creating a contribution and all of the line item objects, eg the membership created when buying a membership, or the event registration when buying a ticket.
In the specification (https://wiki.civicrm.org/confluence/display/CRM/Order+API), the idea was that there would be array of of the parameters required to create line items. Note that although the entity_table is specified, the entity_id is set to null. Moreover, there is no contribution_id value. Calling the Order API is NOT supposed to require prior calls to create other objects.https://lab.civicrm.org/dev/core/-/issues/1380No easy way to import signatures to petition2022-11-30T19:32:57ZCoreyBurgerNo easy way to import signatures to petitionSo there is no easy way to mass import signatures to a petition.
When looking at Petitions on Campaign > Dashboard > Petitions on the right side under the more option, there should be an option to "import signatures", which should be a...So there is no easy way to mass import signatures to a petition.
When looking at Petitions on Campaign > Dashboard > Petitions on the right side under the more option, there should be an option to "import signatures", which should be a custom version of the Import Activities.
As a workaround, I had to manually lookup how to import it, include the required numeric fields for source_record_id and activity_id.
CiviCRM 5.15 on Wordpresshttps://lab.civicrm.org/dev/financial/-/issues/95Update Payment.create api handling of payment instrument2019-11-01T12:04:03ZeileenUpdate Payment.create api handling of payment instrumentAt the moment the passed in payment_instrument_id will take precedent over the one associated with the relevant processor. I don't think that is how the design was intended and in other places we use the paymnet_processor_id.
Note that...At the moment the passed in payment_instrument_id will take precedent over the one associated with the relevant processor. I don't think that is how the design was intended and in other places we use the paymnet_processor_id.
Note that I think that the Payment.create api should require either the payment_instrument_id or the payment_processor_id.
Logically this is something 'known' when making a payment & not something we should 'guess'. Transitionally we would deprecate NOT passing in one of those values rathe than requiring it.
I do think we should add some handling to Order.create so that specifically when chaining Payment.create it automatically sets some values - this being one.
@JoeMurray @monish.deb @artfulrobothttps://lab.civicrm.org/dev/financial/-/issues/93Agree / implement handling of failure for doPayment/ doRefund / params for do...2019-11-01T11:58:47ZeileenAgree / implement handling of failure for doPayment/ doRefund / params for doRefundCurrently both doPayment and doRefund throw a PaymentProcessorException if there is a failure either in terms of config or in terms of processing. The flow we agreed back when we implemented this in 4.6 is we should be working towards
`...Currently both doPayment and doRefund throw a PaymentProcessorException if there is a failure either in terms of config or in terms of processing. The flow we agreed back when we implemented this in 4.6 is we should be working towards
```
Order.create.... status=pending
try {
$result = $paymentObject->doPayment();
if ($result['payment_status_id') === 1) {
Payment.create.....
}
}
else {
\\ Add code here to add failed payment
}
```
Currently the CRM_Core_Payment doPayment converts failed results for old processors to exceptions and throws them and new processors have been expected to throw exceptions.
Some of the forms implement things similar to the above although in other cases the exception is caught much further from the call to doPayment and in some cases we are still not creating the order before processing the payment.
Note that in this flow a payment that did not fail but was not successful would not result in a payment being created.
@mattwire is proposing that we change this for doRefund and stop throwing exceptions for failed payments (or any types) and instead return a status for failed for user-related reasons and an exception for system related reasons.
The options as I see them are
1) implement the same pattern for doRefund as doPayment
2) implement and extend the pattern - ie add a couple of things we probably would have done if implementing from scratch such as
- a PaymentProcessorException_PaymentFailed Exception which extends the existing PaymentProcessorException & can be throwing when the payment relates to card failure rather than a config error. Calling code can choose to catch these separately if it chooses. (we could use this for doPayment too)
- add a getter for the outcome ie. start a transition from returning 1 / the value that maps to contributionStatus = Completed (which is not actually a payment status) to ```$paymentObject->isCompleted()``` or ``` $paymentObject->isSuccessful()```. (Note Omnipay uses isSuccessful()). We could transition doPayment to this over time.
- add a property / getter for getRefundTrxnResult
- add a property / getter for 'processor_result' / 'passthrough_params' which is something that @mattwire has suggested although I have no specifics / examples.
- encourage the use of getters & setters rather than passing in params - ie ```$paymentObject->setAmount(20.40);``` Notte we have just added getters & setters to the class in general https://lab.civicrm.org/dev/financial/issues/82 - implement this in the api call & new core interactions with the class
3) switch to the pattern matt proposes - ie. for doRefund
```
return [
// refund_status_id would normally return the contribution_status_id Completed or Failed
'refund_status_id' => CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'),
// The refund trxn_id may be different from the trxn_id of the original payment.
// If it is the same then trxn_id should be copied to refund_trxn_id
'refund_trxn_id' => NULL,
// Array of params returned by the payment processor. The contents of this will vary by processor and should not be relied on.
// However, they can be very useful for logging or providing specific feedback.
'processor_result' => [],
];
```https://lab.civicrm.org/dev/financial/-/issues/92Change signature of CRM_Core_Payment::doRefund(&$params) to not receive $para...2019-11-01T12:04:35ZeileenChange signature of CRM_Core_Payment::doRefund(&$params) to not receive $params as a referenceThis is a proposal by @mattwire which is mostly stylistic
pros - we have been moving away from receiving as reference
- less scope for people to try to achieve odd things in there
cons - less consistent with doPayment
- would...This is a proposal by @mattwire which is mostly stylistic
pros - we have been moving away from receiving as reference
- less scope for people to try to achieve odd things in there
cons - less consistent with doPayment
- would need to do a civi universe or whatever that things is of Tims to find what processors might need to be patched to cope
- less opportunity for payment processors to tweak thingshttps://lab.civicrm.org/dev/financial/-/issues/91Discuss / maybe resolve schema issue on changing line items2019-10-25T22:17:31ZeileenDiscuss / maybe resolve schema issue on changing line itemsPer
https://docs.civicrm.org/dev/en/latest/financial/financialentities/
If the line items change then the items financial items have to be updated. Generally the rule is to zero-out the current line items
and reverse their financial ite...Per
https://docs.civicrm.org/dev/en/latest/financial/financialentities/
If the line items change then the items financial items have to be updated. Generally the rule is to zero-out the current line items
and reverse their financial items and create new ones. This is a pretty standard accounting approach.
Unfortunately there are some scenarios where the schema does not permit this which has resulted in
adjustment line items being created in these cases. The issue is that the civicrm_line_item table has a unique index for
entity_table + entity_id + contribution_id + price_field_value_id + price_field_id.
This means that if a line item with no price_field_values (ie a text / enter quantity line item) is altered it is not possible
to create a reversal line a new line within the schema. The same problem occurs when changing a line item with price_field_values
BACK to a price_field_value it previously held. In both these scenarios the work around is to have more than one 'valid' financial_item
against the resulting line item with an 'adjustment' entry - ie an additional financial_item.
I don't have any good answers except maybe add an 'is_current' field & add that to the index. Perhaps the current pain is unavoidable but it feels a bit like an own goal
@JoeMurray @monish.deb @lcdweb @kcristiano @pradeep