Skip to content

Conversation

@elipousson
Copy link

@elipousson elipousson commented Aug 20, 2024

This is probably an over-crowded PR but I wanted to confirm that I hadn't forgotten about our exchange on here back in February @hongooi73. I've been adding methods that I'm trying out for my wrapper sharepointr package to my branch and figured I'd open a PR.

Here are the changes in this PR so far:

I'm marking this PR as a draft because there are several additional changes that I know are still needed:

  • Add documentation for the column definition to ms_list (may require a helper function for validation or help prepping a definition)
  • Expand documentation for the ms_site$get_pages() method (I can't recall the difference between the two API endpoints - since I wrote that part a month or two ago)
  • Add tests for all of the new methods

I can split this up into multiple PRs if needed or make other changes as appropriate.

@elipousson
Copy link
Author

@microsoft-github-policy-service agree

@hongooi73
Copy link
Collaborator

@elipousson I'll probably be posting an update to CRAN in the next week or 2. Is this PR still active? Apologies for not responding to this.

@elipousson
Copy link
Author

@hongooi73 I’d be glad to finish this up so it is ready to merge. I can probably spend some time on it next week. If you have a chance to take a look, any comments or questions are welcome.

@elipousson elipousson marked this pull request as ready for review February 20, 2025 16:19
@hongooi73
Copy link
Collaborator

If you can't get it done in time there's no rush, it can go into another update next month (CRAN has an informal timeout on the frequency of package updates).

@hongooi73
Copy link
Collaborator

All the @noRd comments make it difficult to find the actual code changes, can you give me a quick rundown on where the changes are and what they do?

@hongooi73
Copy link
Collaborator

Cool stuff! Hope figuring out how the AzureGraph machinery works wasn't too hard.

self$do_operation("analytics")
},

get_pages=function(type=c("sitePage", "page"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be called list_pages if it returns multiple pages, or get_page if it returns only 1 page. If the endpoint has the ability to do both, then have both list_pages and get_page methods that return multiple or 1 page respectively

Copy link
Author

Choose a reason for hiding this comment

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

I think you're correct that it should be list_pages() - it looks like get_page would require the user to provide a page ID: https://learn.microsoft.com/en-us/graph/api/sitepage-get?view=graph-rest-1.0&tabs=http

Since this is offering the option between two API endpoints, should it stay as a single method? Or get split between a list_pages() and list_site_pages() method? I need to look closer at the docs to recall the difference which could inform whether it makes sense to keep them separate or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had a closer look at the docs, and it seems there's a difference between a base site page object and a site page (which inherits from base site page). The base site page uses endpoint URLs of the form sites/*/pages/{ID} and the latter uses sites/*/pages/{ID}/microsoft.graph.sitePage

Which one are you using in your interactions with Sharepoint? Do you know what the differences are in practice?

Copy link
Collaborator

@hongooi73 hongooi73 Feb 27, 2025

Choose a reason for hiding this comment

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

For both endpoints though the schema looks the same as other Graph API endpoints for working with files, emails, etc. The R-side code should follow the same layout as for those as well.

  • Create a 'ms_page' class similar to ms_drive_item or ms_email, inheriting from ms_object
  • The list_pages and get_page methods should return object(s) of this class
  • Add a ms_site$delete_page method that calls down to ms_page$delete() (which should be auto-generated for you)
  • Optionally, add ms_site$create_page which creates a new page by calling ms_page$new(). I don't know how useful this is without being able to generate HTML, so it can be left out.
  • Add any further methods to ms_page that are relevant/necessary for a site page, as well as helper methods for ms_site that call down to these

It's possible that you may need separate ms_page and ms_site_page classes but hopefully that won't be necessary.

Ping me if you need help on these; the drive item and email classes should help explain how it works.

Copy link
Author

Choose a reason for hiding this comment

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

That sounds like a good scope but I think I'd prefer to do all that in a separate PR. I'll remove the get_pages method from this PR when I get a chance to sit down and knock out the other outstanding issues.

Copy link
Collaborator

@hongooi73 hongooi73 Mar 1, 2025

Choose a reason for hiding this comment

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

It's fine to keep it all in one PR, it's basically the one feature. But yeah, the analytics and other stuff could be done elsewhere. I'll also be submitting to CRAN on the weekend, so this can go into the next update for March or later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants