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.

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

Updated profiles' create and edit #180

Merged
merged 6 commits into from May 25, 2020

Conversation

leobragaz
Copy link
Contributor

Description

This PR merge the current MsgCreateProfile and MsgEditProfile into the new
MsgSaveProfile that handles both the situations.
Closes #170 .

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@leobragaz leobragaz added kind/enhancement Enhance an already existing feature; no "New feature" to add Status:R4R x/profiles Module that allows to create and manage decentralized social profiles labels May 25, 2020
@leobragaz leobragaz added this to the v0.6.1 milestone May 25, 2020
@leobragaz leobragaz self-assigned this May 25, 2020
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

The implementation looks overall good to me. There's only a small change I would do inside the CLI save command

Comment on lines 63 to 68
picture := viper.GetString(flagProfilePic)
cover := viper.GetString(flagProfileCover)
moniker := viper.GetString(flagMoniker)
name := viper.GetString(flagName)
surname := viper.GetString(flagSurname)
bio := viper.GetString(flagBio)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if using get GetString having an empty string works as we would want to. What I was thinking is the following.

Suppose you execute desmoscli tx save --name="Riccardo" then what will happen is that all the remaining fields are rest to "" instead of nil (I've checked and they are two different values). So I think we should change it appropriately.

This could be done by creating the following functions:

// getOrNil returns a pointer to the given value if it is not empty. Otherwise it returns nil
func getOrNil(value string) *string {
  if len(strings.TrimSpace(value)) == 0 {
    return nil
  }
  return &value
}

And use such function as

moniker := getOrNil(viper.GetString(flagMoniker))
name := getOrNil(viper.GetString(flagMoniker))

x/profile/client/cli/tx.go Outdated Show resolved Hide resolved
@RiccardoM RiccardoM added this to Review in progress in Desmos via automation May 25, 2020
renamed rest handler
Copy link
Contributor Author

@leobragaz leobragaz left a comment

Choose a reason for hiding this comment

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

re-reviewed

@leobragaz leobragaz requested a review from RiccardoM May 25, 2020 13:04
@codecov
Copy link

codecov bot commented May 25, 2020

Codecov Report

Merging #180 into master will decrease coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
- Coverage   83.12%   82.68%   -0.44%     
==========================================
  Files          58       58              
  Lines        2447     2374      -73     
==========================================
- Hits         2034     1963      -71     
+ Misses        356      355       -1     
+ Partials       57       56       -1     

@RiccardoM RiccardoM merged commit c87d64d into master May 25, 2020
Desmos automation moved this from Review in progress to Done May 25, 2020
@RiccardoM RiccardoM deleted the leonardo/profile-edit-improvements branch May 25, 2020 14:04
RiccardoM pushed a commit that referenced this pull request May 26, 2020
See PR #180

(cherry picked from commit c87d64d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Enhance an already existing feature; no "New feature" to add x/profiles Module that allows to create and manage decentralized social profiles
Projects
No open projects
Desmos
  
Done
Development

Successfully merging this pull request may close these issues.

Change how the edit of the account is performed
2 participants