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

Skip setting DefaultColumnValues if there are none specified in the provisioning template #840

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

jackpoz
Copy link
Contributor

@jackpoz jackpoz commented Feb 16, 2023

Skip setting DefaultColumnValues if there are none specified in the provisioning template.
This avoids adding an unused "LocationBasedMetadataDefaultsReceiver ItemAdded" event receiver (that is currently causing issues on some tenants).

@heinrich-ulbricht
Copy link
Contributor

heinrich-ulbricht commented Feb 19, 2023

I like the idea, but... so far you could clear currently set default values by specifying none in the provisioning template. How would you clear them now?

The resulting behavior might also be inconsistent with other places. Setting default values is also called in other contexts as well.

And why is there a List.cs > SetDefaultColumnValuesAsync and ListExtensions.cs > SetDefaultColumnValuesImplementation and both look similar?

Edit: AH I'm confused. One is PnP.Framework, one PnP.Core!

For the time being I wrapped both implementations in try-catch, log the error, and ate the exception. This solves it for my specific case but is not suited for general use. Or is it?

@jackpoz
Copy link
Contributor Author

jackpoz commented Feb 19, 2023

@heinrich-ulbricht thanks for the feedback.
I would say in general PnP templates and PnP framework ObjectHandlerBase work incrementally:

  • Having a folder in a template will only try to create that, it will not try to delete existing folders
  • Having a site column/list column will only try to create them, not delete existing columns
  • Having a list will only try to create that, not delete existing lists

Following this pattern, applying a PnP template that calls ObjectListInstance shouldn't be the way to remove existing DefaultColumnValues. That should be achieve with an explicit call to Clear-PnPDefaultColumnValues or to the related PnP Framework function. In other cases this is handled with a property in the PnP Provisioning Schema, like RemoveExistingNodes for Navigation, set to true/false. Handling different scenarios requires to add new fields to the PnP Provisioning Schema.

I find it also not optimal to try to always add an event receiver triggered OnAdded even if it's unnecessary as there are no default column values to be set.

Before https://github.com/pnp/pnpframework/pull/576/files#diff-7ad2ebe8433e6e546bf24c3e8a6972fabe57c838762b8b85cd086823b2903f7cL1659 calling SetDefaultColumnValuesImplementation() with no columns would have done nothing and just returned, so in a way this PR restores the behavior was existed until February 2022.

@heinrich-ulbricht
Copy link
Contributor

heinrich-ulbricht commented Feb 19, 2023

@jackpoz So far I was under the impression that not specifying any default column values in the PnP template would trigger existing default values to be cleared:

private static void SetDefaultColumnValuesImplementation(this List list, IEnumerable<IDefaultColumnValue> columnValues)
{
if (columnValues == null || !columnValues.Any())
{
list.ClearDefaultColumnValues();
}

That's also why the engine tries to do something with default values even if the PnP template does not say anything about default values.

I agree that this is unexpected and undesired. But that seems to be the way it worked until it broke recently. Not sure though about the history.

If the described behavior is something that has been introduced recently then it probably is worth reverting it to what you describe: no default values in the template => no resetting by the engine.

jansenbe added a commit that referenced this pull request Feb 22, 2023
@jansenbe jansenbe merged commit c4fd146 into pnp:dev Feb 22, 2023
@anthonywhite
Copy link

@jackpoz do you know if "the problem in some tenants" shows itself as a console error Error applying default column values? If so this could be the issue we are seeing in #846 ... do you know of any workaround/change we can do in our template to make it work, pending this change being rolled out?

@jackpoz
Copy link
Contributor Author

jackpoz commented Feb 25, 2023

the error shows as "Error applying default column values", as that's the exception message that PnP Framework throws.
You can see how to reproduce steps at pnp/powershell#2814 (comment) and at pnp/powershell#2814 (comment)

Please note that this PR doesn't not fix that issue, as that's a SPO issue that can be triggered by CSOM (so not PnP). This PR just doesn't invoke the code path that would trigger that error unless required, which IMO is the most common case. I have some customers that do use DefaultColumnValues and for those the only solution is to open a support ticket.

As for the "do you know of any workaround/change we can do in our template to make it work, pending this change being rolled out?" , PnP Framework has nightly builds published to nuget at https://www.nuget.org/packages/PnP.Framework/ and PnP Powershell does the same at https://www.powershellgallery.com/packages/PnP.PowerShell .That means there are already versions ready to be used that include this PR, even if as nightly builds.

@anthonywhite
Copy link

@jackpoz many thanks for the quick reply.

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.

None yet

4 participants