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

dev/core#1374 Fix search formValue handling on contribution search #15781

Merged
merged 1 commit into from
Nov 9, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

This fixes 2 issues

  1. Search params being lost when editing a related entity per
    https://lab.civicrm.org/dev/core/issues/1374
  2. force=1&sort_name=p not working in contribution search url

Before

"On dmaster just now, I searched for contributions with amount from 800 to 800, got 18 results. Edited one of them and saved. Listing now shows all contributions (93). Opening Search criteria I see that the 800 to 800 criteria are still there. Clicking search button gets back original search results of 18 records.
Seems like when closing edit form, there is a refresh of search, but the refresh is missing the criteria. IIRC, there was a recent fix where only the edited item showed up after saving the edit form. So possible regression. This is an irritation rather than crucial functionality."

Also adding force=1&sort_name=p does not work in contribution search

After

Above fixed

Technical Details

In digging I concluded the problem is we have 3 underlying arrays which we keep jumbling together

  1. formValues - the actual submitted values, augmented by any url passed params
  2. the default values - values to load by default on the form
  3. our working query params - a copy of formValues that we have prepared for the query

We need to stop mangling them. I added subtle code comments

Comments

@seamuslee001 enough has happened between 5.19 & 5.20 I think it might be fine to put this in the rc but not port to 5.19

@civibot
Copy link

civibot bot commented Nov 8, 2019

(Standard links)

@civibot civibot bot added the 5.20 label Nov 8, 2019
@seamuslee001
Copy link
Contributor

@eileenmcnaughton style fix issue

@seamuslee001
Copy link
Contributor

I was able to replicate the issue on 5.20 and confirmed that both are fixed. I feel like getting 1 sorted in 5.19 would be a nice to have but not a necessity I would also add that it looks like contribution_amount_low is not being passed in right when in force mode in a search url like

civicrm/contribute/search?reset=1&force=1&sort_name=allan&contribution_amount_low=75

@seamuslee001
Copy link
Contributor

@eileenmcnaughton i think the test fail relates here

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 we probably want to change contribution_amount_low=75 to be total_amount_low=75 since that would be std. I don't know when it did work but ... master for that?

This fixes 2 issues
1) Search params being lost when editing a related entity per
https://lab.civicrm.org/dev/core/issues/1374
2) force=1&sort_name=p not working in contribution search url

In digging I concluded the problem is we have 3 underlying arrays which we keep jumbling together

1) formValues - the actual submitted values, augmented by any  url passed params
2) the default values - values to load by default on the form
3) our working query params - a copy of formValues that we have prepared for the query

We need to stop mangling them. I added subtle code comments
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 ok - I think it's legit to change the test set up here

@seamuslee001
Copy link
Contributor

ok added Merge on Pass

@seamuslee001 seamuslee001 merged commit a4b70c3 into civicrm:5.20 Nov 9, 2019
@seamuslee001 seamuslee001 deleted the cont_search branch November 9, 2019 00:37
@mfb
Copy link
Contributor

mfb commented Nov 20, 2019

Backporting this to 5.19 could be helpful, as paging thru contribution search is borked (shows all results).. but yes, doesn't apply cleanly...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants