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

allow event transfer on backend when self-service transfer is disabled #20150

Merged
merged 1 commit into from
May 5, 2021

Conversation

MegaphoneJon
Copy link
Contributor

https://lab.civicrm.org/dev/event/-/issues/54

Overview

When self-service transfer/cancellation is disabled, back-office transfer/cancellation is also disabled.

Before

Error message: "This event registration can not be transferred or cancelled. Contact the event organizer if you have questions."

After

You can reach the transfer/cancellation screen (which has its own bug, but that's out of scope here).

Technical Details

Pretty sure this regressed on event#35 (aka me).

Comments

I refactored a bunch of this code last year, but there's still some very poor code here. If we move the transfer and cancellation functions from the form layer to the BAO that will improve matters considerably.

@civibot
Copy link

civibot bot commented Apr 26, 2021

(Standard links)

@civibot civibot bot added the master label Apr 26, 2021
@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit is this one you could review?

@seamuslee001
Copy link
Contributor

@Edzelopez I think you will be interested in this one

@Edzelopez
Copy link
Contributor

Thanks @seamuslee001

Applied this one and can confirm it does what is necessary to prevent that error from showing up for admins.

@eileenmcnaughton
Copy link
Contributor

@Edzelopez does that also mean you think the code approach is correct? I am not too familiar with this area of code but the change seems trivial enough & it appears logical so I think it's OK to merge based on it having been tested but would prefer input from you @Edzelopez

Adding merge-ready as we should merge pre-forking ideally

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label May 4, 2021
@monishdeb
Copy link
Member

I am going to merge this PR as @Edzelopez and I have reviewed the patch and confirmed that it fixes the regression reported in the ticket. Also the extended unit-test captures the use-case accurately.

@monishdeb monishdeb merged commit ca64a8f into civicrm:master May 5, 2021
@eileenmcnaughton
Copy link
Contributor

thanks @monishdeb

@eileenmcnaughton
Copy link
Contributor

@monishdeb sorry - just reading - I wasn't aware this was a regression - do we need to patch the rc?

@monishdeb
Copy link
Member

Yeah I think so, should I created PR against 5.37 and/or 5.36 ?

@seamuslee001
Copy link
Contributor

@monishdeb I think just PR against 5.37 given new release is dropped tomorrow

@monishdeb
Copy link
Member

Done submitted PR against 5.37 #20223

@lkuttner
Copy link

What is the tests/phpunit/CRM/Event/BAO/ParticipantTest.php file and where should it be? I do not find it anywhere on our server.

@MegaphoneJon
Copy link
Contributor Author

@lkuttner That is part of the CiviCRM test suite, and is only present if you set up CiviCRM in a development context. It's not present in the downloadable versions of CiviCRM, and is unnecessary for normal use. If you're trying to apply this patch you can just ignore that change - its purpose is to ensure that future changes won't cause this bug to reappear.

@MegaphoneJon MegaphoneJon deleted the backoffice-xfer branch September 13, 2021 17:42
@lkuttner
Copy link

lkuttner commented Sep 13, 2021

Ah, OK. Thanks @MegaphoneJon. I did patch the Participant.php and do a couple of tests. I don't get the restriction any more, however a transfer results in the creation of a copy of the transferred registration for the new participant, and the original participant is still registered with the status of Registered. This is on 5.33.5 running on Drupal 7.8.2.

@MegaphoneJon
Copy link
Contributor Author

@lkuttner Do you have a test site? I feel like there were a number of fixes in this area since 5.33, and if you can upgrade a test site to 5.41, it could help identify if there's anything else you need.

@lkuttner
Copy link

Yes, our test server is currently is on 5.39.0. I just patched and tested on that, and registrations did transfer properly, so your suspicion was correct. What regular release is/will this update be included in? Thanks!

@MegaphoneJon
Copy link
Contributor Author

This particular patch was included in 5.38 - but there must be another patch that is also relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants