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

GetComments() only return maximum of 30 comments. It used to return all comments. #1361

Closed
1 task done
larry-lau opened this issue Jan 29, 2024 · 6 comments
Closed
1 task done
Assignees
Labels
area: model 📐 Related to the core SDK models bug Something isn't working

Comments

@larry-lau
Copy link

Category

  • Bug

Describe the bug

GetComments() used to return all comments. Now it is only returning 30 comments.

Steps to reproduce

  1. See code block below
  2. Add the following unit test to the PagesTests.cs in PnP.Core.Test project
  3. Run the test without mocking turned off.
  4. Assertion failure since it returns 30 comments instead of 45 that get added in this test.
[TestMethod]
public async Task PageCommentingTestOnlyReturn30Comments()
{
    TestCommon.Instance.Mocking = false;
    using (var context = await TestCommon.Instance.GetContextAsync(TestCommon.TestSite))
    {
        var newPage = await context.Web.NewPageAsync();
        string pageName = TestCommon.GetPnPSdkTestAssetName("PageCommentingTest.aspx");

        // Save the page
        await newPage.SaveAsync(pageName);

        // Publish the page, required before it can be liked
        newPage.Publish();

        // Get Page comments                
        var comments = newPage.GetComments();
        Assert.IsTrue(comments.Length == 0);

        var noCommentsAdded = 45;

        foreach (var i in Enumerable.Range(1, noCommentsAdded))
        {
            // Add a comment
            comments.Add($"Comment #: {i} added by unit test");
        }

        comments = newPage.GetComments();
        // Expecting 45 but only 30 is returned. 
        Assert.IsTrue(comments.Length == noCommentsAdded);

        // Delete the page
        await newPage.DeleteAsync();
    }
}

Expected behavior

GetComments() should return all comments like it used to.

Environment details (development & target environment)

  • SDK version: 1.11.0
  • OS: [e.g. Windows 10 | MacOS 10.15.x]
  • SDK used in: [Console App]
  • Framework: [.NET 7/8 ]
  • Browser(s): [N/A]
  • Tooling: [Visual Studio 2021]
  • Additional details: **

Additional context

Thanks for your contribution! Sharing is caring.

@jansenbe
Copy link
Contributor

@larry-lau : from when did this we limit the amount of comments to 30?

@jansenbe jansenbe self-assigned this Jan 30, 2024
@jansenbe jansenbe added question Further information is requested area: model 📐 Related to the core SDK models labels Jan 30, 2024
@larry-lau
Copy link
Author

@larry-lau : from when did this we limit the amount of comments to 30?

@jansenbe I am not 100% sure. I am looking at the pnpcore code and it is just making HTTP GET on the SharePoint REST API _api/web/lists(guid'{List.Id}')/getitembyid({Id})/getcomments and I didn't see any limit imposed there. I am curious if it should have pagination logic to fetch all comments.

@jansenbe
Copy link
Contributor

@larry-lau : or some server side change. Can you capture the REST query and see if it returns all items when you invoke it directly?

@larry-lau
Copy link
Author

larry-lau commented Jan 31, 2024

@jansenbe: The response doesn't returns all items but it does return odata.nextLink which we could use to fetch more data https://xxx.sharepoint.com/sites/TestNews1Edited/_api/web/lists(guid%27aec597d6-13ea-4f08-9b41-a03c19200281%27)/getitembyid(8)/getcomments?%24skiptoken=AAAAABAAAAAAAAAA8FapEvIg3Eg1

I am curious if the pnpcore sdk could support following the odata.nextLink to fetch all comments.

@jansenbe
Copy link
Contributor

@larry-lau : I've reprod this at my side as well (thanks for sharing the test case!). Will see if I can get this one fixed.

@jansenbe jansenbe added bug Something isn't working and removed question Further information is requested labels Feb 15, 2024
@jansenbe
Copy link
Contributor

@larry-lau : and the issue is fixed, fix will be next nightly, so closing this now.

Seems as the API implementation changed and now uses paging by 30 items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: model 📐 Related to the core SDK models bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants