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

Add Set-PnPComplianceTagOnBulkItems cmdlet #3599

Merged

Conversation

wilecoyotegenius
Copy link
Contributor

@wilecoyotegenius wilecoyotegenius commented Nov 24, 2023

Type

  • Bug Fix
  • New Feature
  • Sample

What is in this Pull Request ?

This PR adds a new cmdlet, Set-PnPComplianceTagOnBulkItems, which allows setting/clearing retention labels (aka compliance tags) from to one or more list items.

@KoenZomers
Copy link
Collaborator

@wilecoyotegenius Nice one. Not a huge fan of the cmdlet name though. I see how this resembles the name it has been assigned in CSOM, but to me it would not be clear what it does by looking at the name. We already have a Set-PnPLabel which sets a label on a list. What if we add an alias on that one to be called Set-PnPSensitivityLabel so we can deprecate its current name in a future (major) version and merge your your code in that one. By using a paramset we can make it work for both the entire list as well as individual list items.

What do you think? Would that make sense?

@wilecoyotegenius
Copy link
Contributor Author

@KoenZomers This cmdlet does not set sensitivity labels but retention labels on items, so alias Set-PnPSensitivityLabel would be totally misleading.
I am perfectly fine with adding a paramset to Set-PnPLabel.

@wilecoyotegenius
Copy link
Contributor Author

I can implement the paramset, if needed. Just let me know :)

@KoenZomers
Copy link
Collaborator

Ah true.. there we go.. misleading naming :) What about renaming to Set-PnPRetentionLabel and adding it to that?

@wilecoyotegenius
Copy link
Contributor Author

Perfectly fine for me.

@KoenZomers
Copy link
Collaborator

Go for it! Let me know if you need help.

Rename Reset-PnPLabel to Reset-PnPRetentionLabel
Rename SetPnPLabelTests to SetPnPRetentionLabelTests
Rename ResetPnPLabelTests to ResetPnPRetentionLabelTests
Remove Set-PnPComplianceTagOnBulkItems
Move logic from Set-PnPComplianceTagOnBulkItems to Set-PnPRetentionLabel and Reset-PnPRetentionLabel
@wilecoyotegenius
Copy link
Contributor Author

@KoenZomers Commit pushed. I have renamed Set-PnPLabel to Set-PnPRetentionLabel and Reset-PnPLabel to Reset-PnPRetentionLabel and distributed the logic from Set-PnPComplianceTagOnBulkItems to cmdlets.

I have not renamed Get-PnPLabel in the same fashion, as there already exists a Get-PnPRetentionLabel cmdlet with a different logic. Get-PnPRetentionLabel returns all retention labels from whole tenant, whereas Get-PnPLabel returns retention label set for a list.

As Get-PnPLabel would now have inconsistent naming, this could result in people running Get-PnPRetentionLabel instead of Get-PnPLabel to fetch a retention label for a list. In such a case, they'll get wrong results.

The solution might be to combine Get-PnPLabel and Get-PnPRetentionLabel and use a paramset to execute either logic to either fetch all retention labels from a tenant (like current Get-PnPRetentionLabel does) or label for a specific list (done by current Get-PnPLabel).

Let me know, what do you think about this idea and I would implement this as well.

@wilecoyotegenius
Copy link
Contributor Author

One more idea - instead of combining cmdlets, we could leave them separate but rename current Get-PnPRetentionLabel to Get-PnPTenantRetentionLabel and then rename current Get-PnPLabel to Get-PnPRetentionLabel.
Leaving cmdlets separate might be reasonable due to the fact that currently code of Get-PnPRetentionLabel and Get-PnPLabel is in different namespaces (Purview and InformationManagement respectively).

@wilecoyotegenius
Copy link
Contributor Author

@KoenZomers Shall we continue with this PR?

@KoenZomers
Copy link
Collaborator

Apologies for the delay in my response. I like your proposal. I have updated the PR accordingly and will merge it. Thanks for your contribution!

@KoenZomers KoenZomers merged commit 8b129ab into pnp:dev Feb 9, 2024
3 checks passed
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