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.

Sign up for GitHub

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

Jump to bottom

(dev/core#491) Standardise the adding of campaign fields on the maili… #13383

Merged
merged 1 commit into from Mar 13, 2019
Merged

(dev/core#491) Standardise the adding of campaign fields on the maili… #13383

merged 1 commit into from Mar 13, 2019

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jan 2, 2019 •
edited by eileenmcnaughton

…ng summary report and add a post upgrade message about needing to re-save report given the changes made to the report template

Overview

This standardises the way campaign fields are added to the mailing summary report and removes a join from the report

Before

Campaign fields were in a non standardard way

Screenshot 2019-03-12 10 28 59

After

Campaign fields added in standard way
Screenshot 2019-03-12 10 31 30

@eileenmcnaughton @yashodha this one i felt needed a post upgrade message because we are actually changing the field names here and changing the way the report outputs in a greater way than the others

@civibot
Copy link

civibot bot commented Jan 2, 2019

(Standard links)

@civibot civibot bot added the master label Jan 2, 2019
@seamuslee001 seamuslee001 force-pushed the report_cleanup_mailing_summary branch 2 times, most recently from c2afb10 to 9d6dcb8 Compare January 2, 2019 21:41
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@yashodha
Copy link
Contributor

yashodha commented Jan 3, 2019

@seamuslee001
Shouldn't this be civicrm_mailing intead of civicrm_membership?

@seamuslee001
Copy link
Contributor Author

thanks @yashodha have fixed and moved the upgrade to the 5.11 upgrade file

@seamuslee001
Copy link
Contributor Author

@yashodha does this look good to you now?

@yashodha
Copy link
Contributor

yashodha commented Jan 9, 2019

@seamuslee001 thanks! I will take a look

@eileenmcnaughton eileenmcnaughton added this to Main Review to-do-list in review board Feb 25, 2019
@seamuslee001
Copy link
Contributor Author

ping @yashodha can you review this?

@eileenmcnaughton
Copy link
Contributor

I think this results in a minor loss of functionality - ie. you can't do 'contains' criteria - but if that was a problem for someone we could alter the parent function to make contains generally available in these reports. OTOH it's likely no-one will ever care.

If you move the Upgrade comments @seamuslee001 I'll merge this in the interests of completing this campaign tidy up

@seamuslee001
(dev/core#491) Standardise the adding of campaign fields on the maili…
8a4cd1f 
…ng summary report and add a post upgrade message about needing to re-save report given the changes made to the report template

Update language on postupgrade message

Move upgrade to 5.13
@seamuslee001 seamuslee001 force-pushed the report_cleanup_mailing_summary branch from c151c63 to 8a4cd1f Compare March 12, 2019 21:18
@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton done

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Mar 12, 2019 •
edited

merge on pass. I have a tiny misgiving about the functionality change but I can see this is the last step in a big cleanup on this field that reduces our maintenance burden & it feels like an obscure difference

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

3 similar comments
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

Merging as per the tag

@seamuslee001 seamuslee001 merged commit 1d997b4 into civicrm:master Mar 13, 2019
Product maintenance automation moved this from To Review to Pending Merge Mar 13, 2019
review board automation moved this from Main Review to-do-list to done Mar 13, 2019
@seamuslee001 seamuslee001 deleted the report_cleanup_mailing_summary branch March 13, 2019 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yashodha yashodha Awaiting requested review from yashodha

Assignees
No one assigned
Labels
master merge on pass ok-without-test
Projects
Product maintenance
  
Pending Merge
Milestone
No milestone
3 participants
@seamuslee001 @yashodha @eileenmcnaughton