-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Conversation
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.
Looks good to me.
I made one small comment, but that's just my personal preference.
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.
Looks good 👍 but after having a play, I think the workflow of enabling the profile needs to be added because now my profile is empty but my links are populated which is confusing
I think we need this steps
- new page with "profile deactivated" message and a button to enable profile
- all manage pages should redirect to new deactivated profile page to avoid confusion
Are we expecting a redirection from statistics tab as well or user can see statistics page with disable profile ? |
Redirect also please, they should not be able to do anything until they re-enable their account |
@eddiejaoude i have made all the changes, let me know if we can make any more improvements in this feature. |
Looks good to me |
pages/account/manage/events.js
Outdated
} | ||
|
||
//reroute to profile deactivated page if profile is disabled | ||
if(profile && profile.hasOwnProperty("isEnabled") && !profile["isEnabled"]) { |
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.
!profile["isEnabled"]
is not the coding style we generally use
I think this could be simplified to
if(profile && profile.hasOwnProperty("isEnabled") && !profile["isEnabled"]) { | |
if(profile && !profile.isEnabled) { |
headers: { | ||
"Content-Type": "application/json", | ||
}, | ||
body: JSON.stringify({ name, bio, tags, layout, isEnabled: false}), |
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.
if doing a delete, I don't think all the other data fields are required
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.
Using all the data fields could this be simplified by re-using the handleSubmit
function?
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.
if doing a delete, I don't think all the other data fields are required
https://github.com/EddieHubCommunity/LinkFree/blob/32fb2bd52bc71791a169458eaa96a2b67d780b91/pages/api/account/manage/profile.js#L60
while creating the updateProfile
object , we are completely relying on req.body
data. we can also perform findOne call using _id in API but it doesn't felt appropriate to add a new DB call for this. so i thought it's a good idea to send all data.
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.
Using all the data fields could this be simplified by re-using the
handleSubmit
function?
Are you looking for a approach similar to this ?
const deleteProfile = async() => {
handleSubmit(false); // false is isEnabled flag value
return Router.push(`${BASE_URL}/account/profile-deactivated`);
}
This approach for some reason is adding a reload to profile page whenever handleSubmit is called.
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.
I will have a look
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.
Thank you 👍
- I left some inline comments
- When visiting the profile after the delete, it is still reachable - the profile page (api) needs to check the profile is enabled, and if it is
false
to return a 404 - Please can you resolve the conflicts
dc1c066
to
f804068
Compare
Let me know if you still feel anymore changes can be done here, Thanks !! |
Thank you for making the changes, I will take a look 👍 |
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.
I've left a comment about the broken user creation
//reroute to profile deactivated page if profile is disabled | ||
if(profile && !profile.isEnabled) { | ||
return { | ||
redirect: { | ||
destination: "/account/profile-deactivated", | ||
permanent: false, | ||
} | ||
} | ||
} | ||
|
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.
I can't create a new profile since new user is not enabled by default. Additionally, there is a check for if (profile && !profile.isEnabled)
in all account pages, so I can't setup a profile.
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.
Hey how can i test the user creation flow? Generally i connect my account locally by the help of access token generated from github.
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, sorry for the long reply. I use Github OAUth keys for local development. so, i can create a profile, log in and modify it using my github info. here's more info about it https://www.biodrop.io/docs/environments/environment-variables
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.
we are specifying isEnabled
flag default value as true
in Profile
schema, so ideally it should inject default value for a new user. not sure why is this going wrong. i am kind of stuck at this issue, can you please guide me anyway possible ?
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.
@webAbhi come and chat to us in EddieHub Discord, we can help better there
Due to lack of activity I am going to close this PR. |
Fixes Issue
fixes #8260
Changes proposed
isEnabled
flag.Check List (Check all the applicable boxes)
Screenshots
Note to reviewers