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

[REF] CiviGrant - Switch to writeRecord/deleteRecord + BAO hooks #26594

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

colemanw
Copy link
Member

Overview

Code refactor in CiviGrant BAO to use newer code patterns.

Comments

Probably needs r-run creating and updating grants, esp checking that notes and attachments work correctly.

@civibot
Copy link

civibot bot commented Jun 21, 2023

(Standard links)

@civibot civibot bot added the master label Jun 21, 2023
@monishdeb
Copy link
Member

Jenkins test this please

@monishdeb
Copy link
Member

@colemanw I encountered an issue. I used the test build site http://core-26594-5azma.test-1.civicrm.org:8001 (--account-name=admin --account-pass=MTQAGHbae3F8) and on contact summary's Grant tab, when I simply submit the Grant update form it creates a new grant record each time:
update

On the other hand, the empty grant type on a fresh install of civigrant is another issue that I guess is covered in #25387 .

The rest are working fine, like the note and attachments are saved properly, also tried with grantapplication, did CRUD operation on grant etc.

@colemanw
Copy link
Member Author

colemanw commented Jul 5, 2023

Thank you for reviewing @monishdeb - I've pushed up a fix for that (form submit function was using the old $ids pattern).

@monishdeb
Copy link
Member

Jenkins test this please

@monishdeb
Copy link
Member

Looks good now. I retested on test-build site and it works fine. I am not sure about an intermittent problem, as you can see in the screencast sometimes it takes too much time to save in popup edit/new grant form, but on refresh, I cannot replicate the issue again:
update

I repeated the same on my local but can't replicate it. I think it's just a glitch and did a thorough code review and can't find any possible cause.

@monishdeb
Copy link
Member

Based on my review above, merging this PR.

@monishdeb monishdeb merged commit cbcf2a3 into civicrm:master Jul 7, 2023
@colemanw colemanw deleted the civiGrantUpdate branch July 7, 2023 14:56
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.

2 participants