Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(user-profile): enable saving empty bio to clear user profile field #33927

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

fix(user-profile): enable saving empty bio to clear user profile field #33927

wants to merge 5 commits into from

Conversation

Curious-Goblin
Copy link

Proposed changes

This pull request enables the ability to save an empty bio in the user profile to clear out the bio field, which was previously uneditable if left empty.

  • This fix addresses an issue where users could not save changes if they attempted to clear their bio.
  • Implements validation adjustments to allow empty strings in the bio field.

Issue(s)

Steps to test or reproduce

  1. Navigate to the user profile section in the Rocket.Chat application.
  2. Attempt to clear the bio field and save changes.
  3. Confirm that the profile successfully updates with an empty bio field.

Checklist

  • I have read the Contributing Guide.
  • I have signed the CLA.
  • Lint and unit tests pass locally.
  • Added tests to verify this fix.
  • Updated documentation as needed.

@Curious-Goblin Curious-Goblin requested a review from a team as a code owner November 9, 2024 16:37
@dionisio-bot dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 9, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 9, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f3607b3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/network-broker Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Gustrb
Gustrb requested changes Nov 10, 2024
View reviewed changes
.changeset/slimy-lamps-learn.md Outdated Show resolved Hide resolved
apps/meteor/server/methods/saveUserProfile.ts
@@ -79,7 +79,7 @@ async function saveUserProfile(
await setUserStatusMethod(this.userId, settings.statusType as UserStatus, undefined);
}

if (user && settings.bio) {
if (user && settings.bio !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add tests to ensure this behavior?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I have added the test case to check the setting of an empty bio field as requested in the commit #bf964f3 .Please let me know if there’s anything else you'd like me to adjust.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this rather?

Suggested change
if (user && settings.bio !== undefined) {
if (user) {

the very next line validates string or not already.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I think keeping the settings.bio !== undefined check in place allows users to have more control by ensuring that the bio field is only updated when explicitly provided. This approach prevents any unintentional updates, such as accidentally clearing the bio if the field is omitted from the request. Let me know if you’d like me to adjust this further.

@Curious-Goblin
feat(users): add test for setting empty bio field and patch bio updat…
bf964f3 
…e handling

- Added a test case to ensure the bio field is correctly set to an empty string.
- Updated user identity handling to support clearing the bio field.
debdutdeb
debdutdeb requested changes Nov 11, 2024
View reviewed changes
apps/meteor/app/lib/server/functions/getAvatarSuggestionForUser.ts Outdated Show resolved Hide resolved
apps/meteor/server/methods/saveUserProfile.ts
@@ -79,7 +79,7 @@ async function saveUserProfile(
await setUserStatusMethod(this.userId, settings.statusType as UserStatus, undefined);
}

if (user && settings.bio) {
if (user && settings.bio !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this rather?

Suggested change
if (user && settings.bio !== undefined) {
if (user) {

the very next line validates string or not already.

debdutdeb
debdutdeb requested changes Nov 11, 2024
View reviewed changes
Copy link
Member

@debdutdeb debdutdeb left a comment

Choose a reason for hiding this comment

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

there should be api test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@debdutdeb debdutdeb Awaiting requested review from debdutdeb

@Gustrb Gustrb Awaiting requested review from Gustrb

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot Set Bio to Empty
3 participants
@Curious-Goblin @Gustrb @debdutdeb