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

MM-43780: Adjust getPostThread API for easier pagination #20172

Merged
merged 3 commits into from
May 25, 2022
Merged

MM-43780: Adjust getPostThread API for easier pagination #20172

merged 3 commits into from
May 25, 2022

Conversation

ashishbhate
Copy link
Contributor

Summary

  • Allow pagination using just FromCreateAt, instead of requiring both FromCreateAt and FromPost. This will enable clients to easily paginate to the middle of a thread of posts, without having to load posts before (or after) the given CreateAt.

Ticket Link

Release Note

NONE

@ashishbhate ashishbhate added the 2: Dev Review Requires review by a developer label May 10, 2022
@ashishbhate ashishbhate added this to the v7.0 milestone May 10, 2022
@mm-cloud-bot mm-cloud-bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 10, 2022
michelengelen
michelengelen approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

LGTM ... but please consider adding someone else with more experience as additional reviewer, since I am not too familiar with Go and the codebase

@ashishbhate
Copy link
Contributor Author

LGTM ... but please consider adding someone else with more experience as additional reviewer, since I am not too familiar with Go and the codebase

Yep, this was more like a heads up about the change. I've requested a review from Agniva to review the actual code.

agnivade
agnivade approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Nicely done. Great usage of squirrel.

@ashishbhate ashishbhate added 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review and removed 2: Dev Review Requires review by a developer labels May 10, 2022
@ashishbhate
Copy link
Contributor Author

@jgilliam17 The changes in this PR haven't been implemented on the client side yet, but that shouldn't block the reviewing and merging of this PR. The changes in this PR are supposed to be backwards compatible.
Some smoke testing around the getting posts in a thread should do it for now.

@amyblais
Copy link
Member

@jgilliam17 Kind reminder that v7.0 feature complete is this Wednesday, May 18th.

@jgilliam17
Copy link

/update-branch

@jgilliam17 jgilliam17 added the Setup Cloud Test Server Setup an on-prem test server label May 24, 2022
@jgilliam17
Copy link

/update-branch

@jgilliam17
Copy link

jgilliam17 commented May 25, 2022 •
edited
Loading

E2E report from yesterday shows large number of failures. Updating and running again

@jgilliam17
Copy link

Thanks @ashishbhate
Tested, looks good to merge.

  • Verified/smoke tested threads - no change.
    - E2E report looks good, no PR related failures.

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review Setup Cloud Test Server Setup an on-prem test server labels May 25, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@jgilliam17 jgilliam17 merged commit 2315fcf into master May 25, 2022
@jgilliam17 jgilliam17 deleted the MM-43780-adjust-getPostThread-api branch May 25, 2022 22:18
@amyblais
Copy link
Member

@ashishbhate Is this for 7.0 or 7.1, or does it matter?

@ashishbhate
Copy link
Contributor Author

@ashishbhate Is this for 7.0 or 7.1, or does it matter?

It doesn't matter. The webapp/mobile don't use the changes in this PR yet. The changes in this PR are backwards compatible so it can go in any version.

@amyblais amyblais removed this from the v7.0 milestone May 27, 2022
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 27, 2022
FreedomBen pushed a commit to FreedomBen/mattermost-server that referenced this pull request Jun 14, 2022
@mattermod @FreedomBen
MM-43780: Adjust getPostThread API for easier pagination (mattermost#…
22aaeec 
…20172)

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agnivade agnivade agnivade approved these changes

@michelengelen michelengelen michelengelen approved these changes

@jgilliam17 jgilliam17 jgilliam17 approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ashishbhate @amyblais @jgilliam17 @mm-cloud-bot @agnivade @michelengelen @mattermod