diff --git a/CLAUDE.md b/CLAUDE.md index 724eb0e..6a42fe7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,7 +51,7 @@ This is the core behavior. Funds carry a `fund-type` term and a `designation` po - **`Standard` fund type** → `ucscgiving_link_filter()` (on `post_type_link`) replaces the permalink with `base_url . designation`, sending visitors straight to the external giving form. - **Anything else** (the README calls these "Priority") → keeps its normal permalink and renders `single-fund.php`, where a **block binding** (`ucscgiving/fund-url`, registered in `general.php`) supplies the same computed URL to the "Give" button. -So the same URL is computed by two different paths — `ucscgiving_fund_url()` for the binding, `ucscgiving_link_filter()` for the permalink. Changes to URL construction must be made in both or they will drift. +Both paths compose that URL through **`ucscgiving_build_fund_url( $post_id )`** — the one place it is built. Change URL construction there, not in the callers. The callers differ only in what they do when there is no `base_url`: the binding returns an empty string, the permalink filter returns the unmodified permalink. `tests/php/BuildFundUrlTest.php` has a guard asserting the two agree. `fund-type-term` is an ACF taxonomy field with `return_format = id`, so it yields a term ID that needs `get_term()` to resolve. diff --git a/ROADMAP.md b/ROADMAP.md index 823006c..b1a3302 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -108,7 +108,7 @@ The suite covers functions that are pure once WordPress is stubbed. Anything nee Worth noting the two are entangled: the inlined templates are also the ones carrying the hardcoded production media ([#133](https://github.com/ucsc/ucsc-giving-functionality/issues/133)) and the unpinned template-part references ([#136](https://github.com/ucsc/ucsc-giving-functionality/issues/136)). Whoever picks up the composition question should look at all three together rather than in isolation. -- **Single URL-construction path.** `ucscgiving_fund_url()` (block binding) and `ucscgiving_link_filter()` (permalink) build the same URL independently. Extract one helper so they cannot diverge. Both now have characterization tests (§3), so the extraction can be made and shown to preserve behavior — this is the cheapest remaining item. +- ~~**Single URL-construction path.**~~ Done in [#135](https://github.com/ucsc/ucsc-giving-functionality/issues/135) — both callers now compose the URL through `ucscgiving_build_fund_url()`, with a test asserting they agree. The extraction surfaced that the two had *already* drifted: the binding dropped a designation code of `"0"` where the permalink filter kept it. Unified on the filter's behavior. ## 5. Maintenance diff --git a/lib/functions/general.php b/lib/functions/general.php index 8fb0129..89745d9 100644 --- a/lib/functions/general.php +++ b/lib/functions/general.php @@ -30,6 +30,32 @@ function ucscgiving_register_fund_url_block_binding() { ); } +/** + * Build the external Giving URL for a fund + * + * The single place the Giving URL is composed. Both the block binding and the + * permalink filter go through here so the two cannot drift apart. + * + * `base_url` lives on an ACF options page registered by the ucsc-2022 theme, + * not by this plugin, so it is empty whenever that theme is inactive. This + * returns an empty string in that case and leaves the callers to decide what + * to do about it — they legitimately differ. + * + * @param int $post_id Fund post ID. + * @return string The escaped Giving URL, or an empty string if no base URL is set. + */ +function ucscgiving_build_fund_url( $post_id ) { + $baseurl = get_field( 'base_url', 'option' ); + + if ( empty( $baseurl ) ) { + return ''; + } + + $designation = get_post_meta( $post_id, 'designation', true ); + + return esc_url( $baseurl . $designation ); +} + /** * Get Fund URL * @@ -38,19 +64,7 @@ function ucscgiving_register_fund_url_block_binding() { * @return string */ function ucscgiving_fund_url() { - $baseurl = get_field( 'base_url', 'option' ); - $designation = get_post_meta( get_the_ID(), 'designation', true ); - $fundurl = ''; - - if ( ! empty( $baseurl ) && ! empty( $designation ) ) { - $fundurl = $baseurl . $designation; - } elseif ( ! empty( $baseurl ) ) { - $fundurl = $baseurl; - } else { - $fundurl = ''; - } - - return esc_url( $fundurl ); + return ucscgiving_build_fund_url( get_the_ID() ); } /** @@ -82,15 +96,15 @@ function ucscgiving_link_filter( $post_link, $post ) { return $post_link; } - $baseurl = get_field( 'base_url', 'option' ); + $fund_url = ucscgiving_build_fund_url( $post->ID ); - if ( empty( $baseurl ) ) { + // No base URL means nothing to link out to, so keep the real permalink + // rather than sending visitors to a truncated address. + if ( '' === $fund_url ) { return $post_link; } - $designation = get_post_meta( $post->ID, 'designation', true ); - - return esc_url( $baseurl . $designation ); + return $fund_url; } add_filter( 'post_type_link', 'ucscgiving_link_filter', 10, 2 ); diff --git a/tests/php/BuildFundUrlTest.php b/tests/php/BuildFundUrlTest.php new file mode 100644 index 0000000..65b4ea8 --- /dev/null +++ b/tests/php/BuildFundUrlTest.php @@ -0,0 +1,119 @@ +assertSame( 'https://give.example.edu/fund/ABC123', ucscgiving_build_fund_url( 7 ) ); + } + + /** + * No base URL means no URL at all. Callers decide what to do with that. + * + * @return void + */ + public function test_returns_empty_string_without_base_url() { + UCSCGiving_Test_State::$meta['7:designation'] = 'ABC123'; + + $this->assertSame( '', ucscgiving_build_fund_url( 7 ) ); + } + + /** + * An absent designation leaves the base URL on its own. + * + * @return void + */ + public function test_returns_base_url_when_designation_is_empty() { + UCSCGiving_Test_State::$fields['base_url'] = 'https://give.example.edu/fund/'; + + $this->assertSame( 'https://give.example.edu/fund/', ucscgiving_build_fund_url( 7 ) ); + } + + /** + * A designation of "0" is a real code and must survive. + * + * This pins the one behaviour that changed when the two implementations + * were merged. `ucscgiving_fund_url()` previously guarded with + * `! empty( $designation )`, so a "0" code was silently dropped and the + * bare base URL returned; `ucscgiving_link_filter()` concatenated and + * kept it. The helper now concatenates, so both agree on the filter's + * behaviour, which is the correct one. + * + * @return void + */ + public function test_appends_a_zero_designation() { + UCSCGiving_Test_State::$fields['base_url'] = 'https://give.example.edu/fund/'; + UCSCGiving_Test_State::$meta['7:designation'] = '0'; + + $this->assertSame( 'https://give.example.edu/fund/0', ucscgiving_build_fund_url( 7 ) ); + } + + /** + * The helper reads the post ID it is handed, not loop state. + * + * @return void + */ + public function test_reads_the_post_id_it_is_given() { + UCSCGiving_Test_State::$fields['base_url'] = 'https://give.example.edu/fund/'; + UCSCGiving_Test_State::$meta['7:designation'] = 'ABC123'; + UCSCGiving_Test_State::$meta['8:designation'] = 'WRONG'; + UCSCGiving_Test_State::$current_post_id = 8; + + $this->assertSame( 'https://give.example.edu/fund/ABC123', ucscgiving_build_fund_url( 7 ) ); + } + + /** + * The drift guard this refactor exists for. + * + * The block binding and the permalink filter must produce the same URL + * for the same fund. Before the helper they computed it independently, + * and a change to one could silently leave the other behind. + * + * @return void + */ + public function test_binding_and_permalink_agree_for_a_standard_fund() { + UCSCGiving_Test_State::$fields['base_url'] = 'https://give.example.edu/fund/'; + UCSCGiving_Test_State::$fields['fund-type-term'] = 99; + UCSCGiving_Test_State::$terms[99] = new WP_Term( 99, 'Standard' ); + UCSCGiving_Test_State::$meta['7:designation'] = 'ABC123'; + UCSCGiving_Test_State::$current_post_id = 7; + + $post = (object) array( + 'ID' => 7, + 'post_type' => 'fund', + ); + + $this->assertSame( + ucscgiving_fund_url(), + ucscgiving_link_filter( 'https://example.test/fund/some-fund/', $post ) + ); + } +}