-
Notifications
You must be signed in to change notification settings - Fork 26
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
🐛 user get consumables if having ♻️ refactor of mentorshipService #1587
🐛 user get consumables if having ♻️ refactor of mentorshipService #1587
Conversation
…e plan, consumables use will be the one's from the paid plan
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…ess to workshop if not subscribed
balance: { | ||
unit: service?.academy?.available_as_saas === false ? -1 : relatedConsumables?.balance?.unit, | ||
unit: (service?.academy?.available_as_saas === false || isAvailableForConsumables === false) ? -1 : relatedConsumable?.balance?.unit, |
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.
It has extra non-necessary validations, if the service.academy.available_as_saas === true then you should always check their consumables, if it's false, then you shouldn't check them because there are no existing consumables for non-saas academies.
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.
When coming from the "choose-program" page, services are assigned the available_as_saas key, as seen here:
const getServices = async (userRoles) => {
if (userRoles?.length > 0) {
delete axios.defaults.headers.common.Academy;
const mentorshipPromises = await userRoles.map((role) => bc.mentorship({ academy: role?.academy?.id }, true).getService()
.then((resp) => {
const data = resp?.data;
if (data !== undefined && data.length > 0) {
return data.map((service) => ({
...service,
academy: {
id: role?.academy.id,
available_as_saas: role?.academy?.available_as_saas,
},
}));
}
return [];
}));
const mentorshipResults = await Promise.all(mentorshipPromises);
const recopilatedServices = mentorshipResults.flat();
setMentorshipServices({
isLoading: false,
data: recopilatedServices,
});
}
};
I think this is necessary because a user could have different services from different cohorts, so we need to differentiate between them. However, when we're already in a specific program, there’s no need to run this function again, as the cohort itself already tells us whether available_as_saas is true or false. That’s why I kept the useEffect
on line 132 in the mentoring
component. however I understand that is not completelly clear so i will change the name cohortSessionIsSaaS
for better clarity.
|
||
let servWithMentorsAvailable; | ||
|
||
if (queryMentor && allMentorsAvailable.length > 0) { |
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.
Avoid nested conditions if possible.
if (allMentorsAvailable.length === 0) {
servWithMentorsAvailable = getAllServicesWithMentors();
return;
}
Using a code like this will make it cleaner and easier to understand the condition
servWithMentorsAvailable = servicesFiltered.filter((service) => mentorFound.services.some((mentServ) => mentServ.slug === service.slug)); | ||
} else { | ||
servWithMentorsAvailable = getAllServicesWithMentors(); | ||
if (!hasReset) { |
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.
You are validating this condition twice, maybe you need this condition to happen before and with a return like shown in the previous comment
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.
This way you won't need to validate this condition on every if statement
if (queryService && servicesWithMentorsAvailable?.length > 0 && shouldHandleService && !hasReset) { | ||
const serviceFound = servicesWithMentorsAvailable.find((service) => service.slug === queryService); | ||
|
||
if (serviceFound) { |
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.
Instead of an if... else...
, only validate the error's condition, the code should look like this:
if(!serviceFound){
toast({
position: 'top',
title: 'Error',
description: `${t('supportSideBar.service-not-found')} "${queryService}" ${queryMentor ? `${t('supportSideBar.for')} "${queryMentor}"` : ''}`,
status: 'error',
duration: 7000,
isClosable: true,
});
return;
}
handleService(serviceFound);
setOpen(true);
setShouldHandleService(false);
Making it cleaner again and easier to understand
@@ -236,20 +295,20 @@ function MentoringConsumables({ | |||
> | |||
|
|||
{open && mentoryProps?.service && ( | |||
<Box position="absolute" top="16px" left="18px" onClick={() => setMentoryProps({})} cursor="pointer"> | |||
<Box position="absolute" top="16px" left="18px" onClick={() => reset()} cursor="pointer"> |
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.
onClick={reset}
src/pages/workshops/[event_slug].jsx
Outdated
@@ -362,6 +390,13 @@ function Page({ eventData, asset }) { | |||
const existsNoAvailableAsSaas = myCohorts.some((c) => c?.cohort?.available_as_saas === false); | |||
const isFreeForConsumables = event?.free_for_all || finishedEvent || (event?.free_for_bootcamps === true && existsNoAvailableAsSaas); | |||
|
|||
useEffect(() => { | |||
if (subscriptionsForCurrentEvent.length === 0) setDenyAccessToEvent(true); | |||
else { |
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.
else setDenyAccessToEvent(false);
Issue: breatheco-de/breatheco-de#7659