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

Add group mailbox export #5153

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Add group mailbox export #5153

merged 4 commits into from
Jan 31, 2024

Conversation

pandeyabs
Copy link
Contributor

@pandeyabs pandeyabs commented Jan 29, 2024 •
edited
Loading

  • Add EML exports for group mailbox.
  • Tested E2E manually along with unit tests added in this PR.
  • Will follow it up with a sanity test PR.

Does this PR need a docs update or release note?

  • ✅ Yes, it's included
  • 🕐 Yes, but in a later PR
  • ⛔ No

Type of change

  • 🌻 Feature
  • 🐛 Bugfix
  • 🗺️ Documentation
  • 🤖 Supportability/Tests
  • 💻 CI/Deployment
  • 🧹 Tech Debt/Cleanup

Issue(s)

  • #

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@aviator-app Aviator app
Copy link
Contributor

aviator-app bot commented Jan 29, 2024 •
edited
Loading

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@pandeyabs pandeyabs self-assigned this Jan 29, 2024
@pandeyabs pandeyabs added export All operations that deal with data export teams & groups Teams and Groups service and API functionality. labels Jan 29, 2024
@pandeyabs pandeyabs marked this pull request as ready for review January 29, 2024 08:16
ryanfkeepers
ryanfkeepers approved these changes Jan 29, 2024
View reviewed changes
Copy link
Contributor

@ryanfkeepers ryanfkeepers left a comment

Choose a reason for hiding this comment

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

some nits; generally lgtm.

src/internal/m365/collection/groups/export.go Outdated Show resolved Hide resolved
src/internal/m365/collection/groups/export.go Outdated Show resolved Hide resolved
src/internal/m365/collection/groups/export.go Outdated Show resolved Hide resolved
src/internal/m365/collection/groups/export.go Outdated Show resolved Hide resolved
ashmrtn
ashmrtn reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@ashmrtn ashmrtn 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.

just skimming and thought I'd leave a note

src/internal/converters/eml/eml.go Outdated
Comment on lines 313 to 315
// TODO(pandeyabs): This is a stripped down copy of messageable to
// eml conversion, it can be folded into one function by having a post
// to messageable converter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming the function names are the same between the Postable and Messageable interfaces, we could just define our won interface that includes the subset of functions that are accessed in this code. If the function names aren't the same between the graph SDK types then we can make a new type that embeds one of the graph SDK types and implements the proper function names we need. It'll just call the corresponding function on the graph SDK type. I think we did something similar for calendar events which have GetCalendar() instead of GetParentFolderId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function names aren't the same. We can make a new type like you said, or convert Postable to Messageable. Either should work out. Tracking this as part of #5155

Base automatically changed from conv_export to main January 30, 2024 21:00
@aviator-app aviator-app bot added the blocked Upstream item prevents completion label Jan 31, 2024
@aviator-app Aviator app
Copy link
Contributor

aviator-app bot commented Jan 31, 2024

This pull request failed to merge: some CI status(es) failed. Remove the blocked label to re-queue.

Failed CI(s): Retention-Test-Suite-Trusted

@pandeyabs pandeyabs removed the blocked Upstream item prevents completion label Jan 31, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 31, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@aviator-app aviator-app bot merged commit 7e2b9da into main Jan 31, 2024
25 checks passed
@aviator-app aviator-app bot deleted the conv_export_1 branch January 31, 2024 05:15
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashmrtn ashmrtn ashmrtn left review comments

@ryanfkeepers ryanfkeepers ryanfkeepers approved these changes

Assignees

@pandeyabs pandeyabs

Labels
export All operations that deal with data export mergequeue teams & groups Teams and Groups service and API functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pandeyabs @ashmrtn @ryanfkeepers