Skip to content

[NEW] Route to get updated roles after a date#16610

Merged
rodrigok merged 11 commits intoRocketChat:developfrom
ashwaniYDV:roleListByUpdatedDate
Mar 20, 2020
Merged

[NEW] Route to get updated roles after a date#16610
rodrigok merged 11 commits intoRocketChat:developfrom
ashwaniYDV:roleListByUpdatedDate

Conversation

@ashwaniYDV
Copy link
Contributor

@ashwaniYDV ashwaniYDV commented Feb 15, 2020 •
edited
Loading

Closes #16599

MarcosSpessatto
MarcosSpessatto suggested changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@MarcosSpessatto MarcosSpessatto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you please create some tests? Here we have an example: https://github.com/RocketChat/Rocket.Chat/blob/develop/tests/end-to-end/api/11-permissions.js#L51-L88

app/api/server/v1/roles.js Outdated
API.v1.addRoute('roles.listByUpdatedDate', { authRequired: true }, {
get() {
check(this.bodyParams, {
updatedAfter: String,
Copy link
Contributor

@MarcosSpessatto MarcosSpessatto Feb 26, 2020

Choose a reason for hiding this comment

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

@ashwaniYDV can you please validate this query parameter, to make sure it is a valid date? Here we have an example: https://github.com/RocketChat/Rocket.Chat/blob/develop/app/api/server/v1/rooms.js#L34-L43

Copy link
Contributor Author

@ashwaniYDV ashwaniYDV Feb 27, 2020

Choose a reason for hiding this comment

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

Done 👍

app/api/server/v1/roles.js Outdated
updatedAfter: String,
});

const roles = Roles.find({ _updatedAt: { $gte: new Date(this.bodyParams.updatedAfter) } }).fetch();
Copy link
Contributor

@MarcosSpessatto MarcosSpessatto Feb 26, 2020

Choose a reason for hiding this comment

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

Can you create a Model method and put this logic inside it to avoid model logic leak?

Copy link
Contributor Author

@ashwaniYDV ashwaniYDV Feb 27, 2020

Choose a reason for hiding this comment

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

Yes sure. I'll do it :)

@ashwaniYDV
Copy link
Contributor Author

ashwaniYDV commented Feb 27, 2020

@MarcosSpessatto I have done the requested changes. Please review

@CLAassistant
Copy link

CLAassistant commented Mar 9, 2020

CLA assistant check
All committers have signed the CLA.

MarcosSpessatto
MarcosSpessatto suggested changes Mar 9, 2020
View reviewed changes
app/models/server/models/Roles.js Outdated
_updatedAt: { $gte: new Date(updatedAfterDate) },
};

return this.find(query, options).fetch();
Copy link
Contributor

@MarcosSpessatto MarcosSpessatto Mar 9, 2020

Choose a reason for hiding this comment

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

I think you can put this fetch outside the method(use when you call the findByUpdatedDate), this way we can call another model methods like count, forEach, etc

@MarcosSpessatto
Copy link
Contributor

MarcosSpessatto commented Mar 9, 2020

@ashwaniYDV can you please add some tests?

@ashwaniYDV
Copy link
Contributor Author

ashwaniYDV commented Mar 10, 2020 •
edited
Loading

@MarcosSpessatto I have merged the two functionalities roles.list and roles.list?updatedSince?date in the same function keeping it consistent to the other APIs.
I have added the tests too.

@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2020

This pull request introduces 1 alert when merging a6b4a42 into 606eb44 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@MarcosSpessatto
Copy link
Contributor

MarcosSpessatto commented Mar 10, 2020

@ashwaniYDV I did some adjustments like keep the two endpoints. Can you please add docs for the new endpoint?

@MarcosSpessatto MarcosSpessatto modified the milestones: 3.0.2, 3.1.0 Mar 10, 2020
@ashwaniYDV
Copy link
Contributor Author

ashwaniYDV commented Mar 10, 2020 •
edited
Loading

hi @MarcosSpessatto
I have updated docs in this repo https://github.com/rocketchat/docs
Please review

@ashwaniYDV ashwaniYDV mentioned this pull request Mar 11, 2020
Merged
@rodrigok rodrigok merged commit ad176ef into RocketChat:develop Mar 20, 2020
ggazzo added a commit that referenced this pull request Mar 21, 2020
@ggazzo
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into dm/m…
f7fe0be 
…ultiple-users

* 'develop' of github.com:RocketChat/Rocket.Chat: (35 commits)
  Change license version requested (#16956)
  Synchronize saml roles to local user (#16158)
  Fix: Padding required in the Facebook Messenger option in Livechat (#16202)
  Add some missing ES translations (#16120)
  Adding margin to click to load text (#16210)
  [FIX] Explicitly set text of confirmation button (#16138)
  Redirected to home when a room has been deleted instead of getting broken link(blank page) of deleted room (#16227)
  Fixed translate variable in ArchiveRoom Modal (#16310)
  [FIX] Display user status along with icon (#16875)
  [FIX] users.setStatus and user from params (#16128)
  Update cypress to version 4.0.2 (#16685)
  [FIX] Text formatted to remain within button even on screen resize  (#14136)
  fix(slack-bridge): messages doesn't send to slack after renaming channel (#16565)
  [NEW] Route to get updated roles after a date (#16610)
  [FIX] Removed Reply in DM from livechat rooms (#16957)
  Update presence package (#16786)
  [NEW] Enterprise Edition (#16944)
  Add an index to the name field for omnichannel department (#16953)
  [FIX] Login with LinkedIn not mapping name and picture correctly (#16955)
  [IMPROVE] Allow login of non LDAP users when LDAP is enabled (#16949)
  ...
ggazzo added a commit that referenced this pull request Mar 21, 2020
@ggazzo
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into dm/m…
82439cf 
…ultiple-users

* 'develop' of github.com:RocketChat/Rocket.Chat: (35 commits)
  Change license version requested (#16956)
  Synchronize saml roles to local user (#16158)
  Fix: Padding required in the Facebook Messenger option in Livechat (#16202)
  Add some missing ES translations (#16120)
  Adding margin to click to load text (#16210)
  [FIX] Explicitly set text of confirmation button (#16138)
  Redirected to home when a room has been deleted instead of getting broken link(blank page) of deleted room (#16227)
  Fixed translate variable in ArchiveRoom Modal (#16310)
  [FIX] Display user status along with icon (#16875)
  [FIX] users.setStatus and user from params (#16128)
  Update cypress to version 4.0.2 (#16685)
  [FIX] Text formatted to remain within button even on screen resize  (#14136)
  fix(slack-bridge): messages doesn't send to slack after renaming channel (#16565)
  [NEW] Route to get updated roles after a date (#16610)
  [FIX] Removed Reply in DM from livechat rooms (#16957)
  Update presence package (#16786)
  [NEW] Enterprise Edition (#16944)
  Add an index to the name field for omnichannel department (#16953)
  [FIX] Login with LinkedIn not mapping name and picture correctly (#16955)
  [IMPROVE] Allow login of non LDAP users when LDAP is enabled (#16949)
  ...
ggazzo added a commit that referenced this pull request Mar 21, 2020
@ggazzo
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into dm/m…
f4a71e8 
…ultiple-users

* 'develop' of github.com:RocketChat/Rocket.Chat: (35 commits)
  Change license version requested (#16956)
  Synchronize saml roles to local user (#16158)
  Fix: Padding required in the Facebook Messenger option in Livechat (#16202)
  Add some missing ES translations (#16120)
  Adding margin to click to load text (#16210)
  [FIX] Explicitly set text of confirmation button (#16138)
  Redirected to home when a room has been deleted instead of getting broken link(blank page) of deleted room (#16227)
  Fixed translate variable in ArchiveRoom Modal (#16310)
  [FIX] Display user status along with icon (#16875)
  [FIX] users.setStatus and user from params (#16128)
  Update cypress to version 4.0.2 (#16685)
  [FIX] Text formatted to remain within button even on screen resize  (#14136)
  fix(slack-bridge): messages doesn't send to slack after renaming channel (#16565)
  [NEW] Route to get updated roles after a date (#16610)
  [FIX] Removed Reply in DM from livechat rooms (#16957)
  Update presence package (#16786)
  [NEW] Enterprise Edition (#16944)
  Add an index to the name field for omnichannel department (#16953)
  [FIX] Login with LinkedIn not mapping name and picture correctly (#16955)
  [IMPROVE] Allow login of non LDAP users when LDAP is enabled (#16949)
  ...
@sampaiodiego sampaiodiego mentioned this pull request Apr 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@MarcosSpessatto MarcosSpessatto MarcosSpessatto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feat: api

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

Route to get updated and deleted roles after a date

4 participants

@ashwaniYDV @CLAassistant @MarcosSpessatto @rodrigok

Comments