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 Extract case action links into a separate function to facilitate refactoring #13793

Merged
merged 1 commit into from
Mar 15, 2019

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Mar 8, 2019

Overview

This is a simple refactor to extract the code that generates the case activity links.

They should be generated using CRM_Core_Action so they use standard CiviCRM markup and pass through hooks etc. but we need to disentangle the code first.

Before

Link generation in shared function

After

Link generation in it's own function, variables specific to generating links are moved to this function. There are a couple of extra things being conditionally set that should be moved out of this function (status_id and no_attachments) but that's for the next round.

Technical Details

Simple function extraction.

Comments

@colemanw As it's case related are you able to take a look? I know there's a lot of improvements that could be made here but I'd like to start with the simple extraction if that's ok and make improvements to the code in subsequent PRs.

@civibot
Copy link

civibot bot commented Mar 8, 2019

(Standard links)

@civibot civibot bot added the master label Mar 8, 2019
@eileenmcnaughton
Copy link
Contributor

@mattwire style issues

@eileenmcnaughton
Copy link
Contributor

Also I agree with extractions NOT being mixed with other changes - that makes them hard to review

@mattwire mattwire force-pushed the case_links_refactor branch from 7543d9c to 830bab4 Compare March 10, 2019 10:31
@mattwire
Copy link
Contributor Author

This branch shows where this might be heading... master...mattwire:case_links_refactor_part2

@eileenmcnaughton
Copy link
Contributor

I worked through this refactor locally and was able to get the same result so I'm going to merge.

But if we are gonna be doing a tidy up in this area I think the next iteration should include tests

@eileenmcnaughton eileenmcnaughton merged commit 08125f8 into civicrm:master Mar 15, 2019
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