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

[FIX] REST API OAuth services endpoint were missing fields and flag to indicate custom services #10299

Merged
merged 4 commits into from
Apr 19, 2018

Conversation

MarcosSpessatto
Copy link
Member

@MarcosSpessatto MarcosSpessatto commented Apr 2, 2018

Add oauth services missing fields and add field to indicate whether the oauth service is customized.
Closes #10298
Closes #10332
@rafaelks , @filipedelimabrito, @cardoso .

@philipbrito
Copy link
Contributor

Thank you @MarcosSpessatto! 👊

buttonLabelText: service.buttonLabelText || '',
buttonColor: service.buttonColor || '',
buttonLabelColor: service.buttonLabelColor || '',
custom: Boolean(service.custom)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you're using Boolean(service.custom) here when we don't use it anywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

@graywolf336 to convert to Boolean if it is some value considered false by JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MarcosSpessatto Please reconsider another way, because if the string is "false" then it will result in a value of true: https://stackoverflow.com/a/264037

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'm going to change the validation, but I don't agree that the string 'false' should be considered Boolean, IMHO 'false' is a string and not a boolean, just as 'true' should be considered a string and not a boolean. So in both cases, casting a Boolean must be true. Why should I consider the string 'false'?

Copy link
Contributor

Choose a reason for hiding this comment

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

You misunderstood what I was saying. If the value of service.custom is a string and the value is 'false' then doing Boolean(service.custom) will result in true being the value instead of the expected false.

Copy link
Contributor

@geekgonecrazy geekgonecrazy Apr 2, 2018

Choose a reason for hiding this comment

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

Just for clarity:
image
a non-empty string in javascript evaluates to true, regardless of content of string

Copy link
Member Author

Choose a reason for hiding this comment

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

The question was, why store in the database a Boolean value as a string and not as Boolean, if you expect it to be Boolean, but @graywolf336 already explained that to me. Thanks.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10299 April 2, 2018 18:09 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10299 April 3, 2018 14:33 Inactive
@sampaiodiego sampaiodiego added this to the 0.64.0 milestone Apr 3, 2018
@MarcosSpessatto MarcosSpessatto changed the title [FIX] Add field to indicate whether the oauth service is customized and change the file [FIX] Add oauth services missing fields, and indicate whether the oauth service is customized Apr 5, 2018
@rafaelks
Copy link
Contributor

Is this one good to go guys?

@rodrigok rodrigok changed the title [FIX] Add oauth services missing fields, and indicate whether the oauth service is customized [FIX] REST API OAuth services endpoint were missing fields and flag to indicate custom services Apr 19, 2018
@rodrigok rodrigok merged commit 45f611e into develop Apr 19, 2018
@rodrigok rodrigok deleted the rest-api-settings-oauth-add-field branch April 19, 2018 02:51
MarcosSpessatto added a commit that referenced this pull request Apr 19, 2018
…-api-chat-postmessage-validations

* commit 'a9fb4da5c847a456990a5d60369f0f52ff4a8bd8': (137 commits)
  Remove "secret" from REST endpoint /settings.oauth response
  [FIX] Directory sort and column sizes were wrong (#10403)
  [FIX] Add oauth services missing fields, and indicate whether the oauth service is customized (#10299)
  Show error message when email verification fails (#10446)
  Correct the column positions in the directory search for users (#10454)
  Fixed custom fields misalignment in registration form (#10463)
  [FIX] Unique identifier file not really being unique (#10341)
  [OTHER] More Listeners for Apps & Utilize Promises inside Apps (#10335)
  [FIX] Empty panel after changing a user's username (#10404)
  [FIX] Russian translation of "False" (#10418)
  [FIX] Links being embedded inside of blockquotes (#10496)
  [FIX] The 'channel.messages' REST API Endpoint error (#10485)
  [OTHER] Develop sync (#10487)
  [FIX] Button on user info contextual bar scrolling with the content (#10358)
  [FIX] "Idle Time Limit" using milliseconds instead of seconds (#9824)
  [NEW] Body of the payload on an incoming webhook is included on the request object (#10259)
  [FIX] Missing i18n translation key for "Unread" (#10387)
  [FIX] Owner unable to delete channel or group from APIs (#9729)
  [NEW] REST endpoint to recover forgotten password (#10371)
  Add REST endpoint chat.reportMessage, to report a message (#10354)
  ...
@rodrigok rodrigok mentioned this pull request Apr 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants