Problem/Motivation

The URL generated in recurly_hosted_account_manage_url() returns a 404. This url is generated using the hosted_login_token property of the Recurly_Account object and matches the format documented in Recurly's API so I am not sure why it is returning 404.

Proposed resolution

This page from the Recurly documentation explains the URL structure needed.

https://docs.recurly.com/docs/hosted-account-management

CommentFileSizeAuthor
#13 interdiff-2869882-10-13.txt1.86 KBmarkdorison
#13 recurly_hosted_account-2869882-13.patch39.04 KBmarkdorison
#10 recurly_hosted_account-2869882-10.patch38.25 KBadamzimmermann
#4 recurly_hosted_account-2869882-4.patch38.37 KBadamzimmermann
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markdorison created an issue. See original summary.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Title: recurly_hosted account url returns 404 » recurly_hosted account manage url returns 404
adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Assigned: Unassigned » adamzimmermann
adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Assigned: adamzimmermann » Unassigned
Status: Active » Needs review
FileSize
recurly_hosted_account-2869882-4.patch38.37 KB

This issue addresses the 404 the module redirects you to when you visit /[recurly-entity]/[entity-id]/subscription/billing. I fixed that, but couldn't find a link to that URL anywhere in the code. I discovered that the route was registered, but no local task was registered for the route. So I set about fixing that, ran into some issues with permissions on the route itself, and also cleaned up the code a bit along the way. This definitely does a bit more, but I believe it is all issues that needed to be fixed.

To test this, visit /user/1/subscription and verify that the "Update Billing" tab appears when recurly_hosted is enabled. Also, verify that it redirects you to a billing update page on recurly.com. A cache clear may be needed to get the tab to appear after applying the patch.

adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 4: recurly_hosted_account-2869882-4.patch, failed testing.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: recurly_hosted_account-2869882-4.patch, failed testing.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented

@adamzimmermann Patch does not apply cleanly.

adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Status: Needs work » Needs review
FileSize
recurly_hosted_account-2869882-10.patch38.25 KB

Fixed.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Needs review » Needs work
Issue tags: +Needs tests

@adamzimmermann

  1. I do not see the "Update billing" tab even after multiple cache clears.
  2. There are a couple code style issues that can be automagically resolved using phpcs/phpcbf. If you don't have phpcs set up on your dev instance, reference the steps defined in .travis.yml which shows how to install the tool and then run the tests.
  3. I would like to add a test or two. Even just something very basic like testing access to the defined routes (see RecurlyJsSettingsFormWebTest for an example).
markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented

@adamzimmermann Updated comment above as after re-testing I now see the tab.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
recurly_hosted_account-2869882-13.patch39.04 KB
interdiff-2869882-10-13.txt1.86 KB
  • Removed unused 'use' statements.
  • Added a test.
walangitan’s picture

walangitan CreditAttribution: walangitan at Chromatic commented
Status: Needs review » Reviewed & tested by the community

#13 applies cleanly and addresses the issue. Marking this RTBC unless there are more tests in mind.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.