-
Notifications
You must be signed in to change notification settings - Fork 35
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
MWPW-133726 add smart redirects for (temporary) client-side redirects #257
Conversation
…e redirects this is a port of adobe/helix-website#671 and adobe/helix-website#670 from www.aem.live to bacom, see discussion in https://adobe.enterprise.slack.com/archives/C0749KS9F8X
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
Commits
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #257 +/- ##
=======================================
Coverage 96.70% 96.70%
=======================================
Files 12 12
Lines 1335 1335
=======================================
Hits 1291 1291
Misses 44 44 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool to see this proceeding, I know it's been discussed a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we port over the unit tests as well, or does that seem unnecessary since this PR points to the location of those tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @trieloff , Thanks for contributing! A few of notes:
- I agree with @JasonHowellSlavin . If there's the possibility of needing maintenance/modifications in this repo, it should probably include the unit tests.
- We usually link a jira ticket in the description. Is there a jira ticket or issue number associated with this feature?
- We can contact the team who manages access or we can create the file for you. Just let us know. Our QE will need something to verify, so we'll want to get that set up before merging.
This slack thread has the entire discussion. Not sure if someone put that into a JIRA ticket.
Like you mentioned, since we are using a different sheet (endpoint), it seems unnecessary to add unit tests for this, since the tests would need a similar test sheet for testing it out anyways.
Whatever is easier for you. If someone can create the file and preview it, we can then test the functionality in this feature branch url. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I've added smart-redirects.json
and included some examples (based on the unit tests) in the description.
Thanks for driving this forward in my absence, @amol-anand |
Release planned for Tuesday 11/5 |
this is a port of adobe/helix-website#671 and adobe/helix-website#670
from www.aem.live to bacom, see discussion in https://adobe.enterprise.slack.com/archives/C0749KS9F8X
Resolves: MWPW-133726
Test URLs
Simple Redirect
One parameter redirect
Multi-parameter redirect
Different order parameter redirect