Skip to content

feat: /lend/* pages server switch-up#3991

Merged
emuvente merged 19 commits into
mainfrom
feature/MARS-165
Oct 17, 2022
Merged

feat: /lend/* pages server switch-up#3991
emuvente merged 19 commits into
mainfrom
feature/MARS-165

Conversation

@christian14b
Copy link
Copy Markdown
Collaborator

@christian14b christian14b commented Jul 7, 2022

This pr makes the following server changes for /lend/* pages :

url server behavior
/lend/id Added in ui redirects to monolith for users that should see old page
/lend/category_name Added in ui new url for /lend-by-category/category_name
/funded/id Removed from routes move this content into borrower profile page so it's all the same route

These changes come along with monolith updates #10728

@christian14b christian14b marked this pull request as ready for review July 7, 2022 19:33
@@ -0,0 +1,553 @@
<template>
Copy link
Copy Markdown
Collaborator

@dyersituations dyersituations Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good way to share this fairly large component? It looks like the only difference from the existing page is the use of WwwPage.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just delete the existing funded borrower profile page.

Comment thread src/router/routes.js Outdated
},
{
path: '/lend/:id(\\d+)',
redirect: '/lend-beta/:id'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this still have pages loading at the /lend-beta/### url? Why not define the BorrowerProfile page component here as well and just use /lend/###...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.. sounds right. @emuvente would this imply setting a canonical url for the Borrower Profile page?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This redirect should be the other way around:

{
	path: '/lend-beta/:id',
	redirect: '/lend/:id'
}

And the route for the borrower profile should be changed from /lend-beta/:id to /lend/:id(\\d+):

{
	name: 'borrowerProfile',
	path: '/lend/:id(\\d+)',
	component: () => import('@/pages/BorrowerProfile/BorrowerProfile'),
	meta: {
		excludeFromStaticSitemap: true,
		unbounce: true,
	},
},

Then there is no need to set a canonical, because the default will be fine.

const queryString = Object.keys(query)
.map(key => `${key}=${query[key]}`)
.join('&');
this.$router.push(`/lend-classic/${this.$route.params.id}?query=${queryString}`);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are some other conditions we'll need to check here...and maybe be able to get rid of this exp cookie check altogether...

  1. If a loan is inPfp we redirect to legacy (https://github.com/kiva/kiva/blob/development/sites/www_kiva/server/views/Xb/Loan/LoanView.php#L95)
  2. If the lender is an RTP Translator we skip the experiment (https://github.com/kiva/kiva/blob/development/httpdocs/classes/API/Experiment/LoanDetailBeta.php#L22)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcstover Sounds good! Is there a way to get if lender has a RTP Volunteer role?

Copy link
Copy Markdown
Contributor

@eddieferrer eddieferrer Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christian14b As of my latest merged PR 1 above is not true. UI will now render inPfp loans

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcstover I think we can ignore those other conditions now. UI will render inPfp loans and volunteers will be directed to /lend-classic from Viva after the monolith pr is live, correct?

@christian14b This check and redirect should happen in the prefetch instead of created. Here's an example of doing a redirect from preFetch:

preFetch(config, client, { route }) {
return client.query({
query: TeamInfoFromId,
variables: {
team_id: numeral(route.query.team_id).value(),
team_recruitment_id: numeral(route.query.id).value(),
team_ids: [numeral(route.query.team_id).value()],
}
}).then(({ data }) => {
const isMember = _get(data, 'my.teams.values', []).length;
// if lender is a member proceed to authenticate/redirect
// this route will handle the redirect to basket/payment or checkout
if (isMember) {
return Promise.reject({
path: '/authenticate/redirect',
query: {
team_id: numeral(route.query.team_id).value(),
promo_id: numeral(route.query.promo_id).value(),
}
});
}
});

Comment thread src/router/routes.js
@@ -0,0 +1,553 @@
<template>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just delete the existing funded borrower profile page.

const queryString = Object.keys(query)
.map(key => `${key}=${query[key]}`)
.join('&');
this.$router.push(`/lend-classic/${this.$route.params.id}?query=${queryString}`);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcstover I think we can ignore those other conditions now. UI will render inPfp loans and volunteers will be directed to /lend-classic from Viva after the monolith pr is live, correct?

@christian14b This check and redirect should happen in the prefetch instead of created. Here's an example of doing a redirect from preFetch:

preFetch(config, client, { route }) {
return client.query({
query: TeamInfoFromId,
variables: {
team_id: numeral(route.query.team_id).value(),
team_recruitment_id: numeral(route.query.id).value(),
team_ids: [numeral(route.query.team_id).value()],
}
}).then(({ data }) => {
const isMember = _get(data, 'my.teams.values', []).length;
// if lender is a member proceed to authenticate/redirect
// this route will handle the redirect to basket/payment or checkout
if (isMember) {
return Promise.reject({
path: '/authenticate/redirect',
query: {
team_id: numeral(route.query.team_id).value(),
promo_id: numeral(route.query.promo_id).value(),
}
});
}
});

Comment thread src/router/routes.js
Comment thread src/router/routes.js Outdated
},
{
path: '/lend/:id(\\d+)',
redirect: '/lend-beta/:id'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This redirect should be the other way around:

{
	path: '/lend-beta/:id',
	redirect: '/lend/:id'
}

And the route for the borrower profile should be changed from /lend-beta/:id to /lend/:id(\\d+):

{
	name: 'borrowerProfile',
	path: '/lend/:id(\\d+)',
	component: () => import('@/pages/BorrowerProfile/BorrowerProfile'),
	meta: {
		excludeFromStaticSitemap: true,
		unbounce: true,
	},
},

Then there is no need to set a canonical, because the default will be fine.

};
},
apollo: {
preFetch(config, client, { cookieStore, route }) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prefetch is going to be called even if the funded borrower profile isn't shown, which will slow down the loading of the page. Instead, this data could be fetched conditionally in the BorrowerProfile prefetch or could be merged with the borrower profile query, and then passed into this component through props.

id="borrower-profile"
>
<article class="tw-relative md:tw-bg-secondary">
<article v-if="amountLeft" class="tw-relative md:tw-bg-secondary">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also check that the loan status is "fundraising", because loans can also have an amount left when they are paying pack.

@eddieferrer
Copy link
Copy Markdown
Contributor

@christian14b I noticed in KivaClassicBasicLoanCard.vue there is a check for this.$route.path.includes('funded'); -- this will have to be refactored since you are removing the funded route

Comment thread src/components/LoanCards/KivaClassicBasicLoanCard.vue
Comment thread src/pages/BorrowerProfile/BorrowerProfile.vue
Comment thread src/components/BorrowerProfile/FundedBorrowerProfile.vue Outdated
Copy link
Copy Markdown
Collaborator

@emuvente emuvente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

path: `/lend-classic/${route.params.id}`,
query,
});
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in the near future we'll be able to get rid of this expCookie stuff and just provide a query param to allow bypass to the legacy bp...or a link in the page.

@emuvente emuvente merged commit 5325838 into main Oct 17, 2022
@emuvente emuvente deleted the feature/MARS-165 branch October 17, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants