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

Extend FieldDefinition property "AddFieldOptions" to array #1091

Open
vlad-ivanov-d opened this issue Oct 20, 2017 · 6 comments
Open

Extend FieldDefinition property "AddFieldOptions" to array #1091

vlad-ivanov-d opened this issue Oct 20, 2017 · 6 comments

Comments

@vlad-ivanov-d
Copy link
Contributor

Hello,

Sometimes it's necessary to use more than one AddFieldOptions property in FieldDefinition (for example: BuiltInAddFieldOptions.AddToAllContentTypes and BuiltInAddFieldOptions.AddFieldInternalNameHint), but right now it's impossible.

So it will be good to create another property with an array type.

Reference to parent issue: #1073

@andreasblueher
Copy link
Contributor

Would you create an array or rather a list to contain those multiple options?

@avishnyakov
Copy link
Contributor

List might be a way forward. We've got both arrays and lists here and there. Array and collection usage seem to be a legacy thing left out there for a backward compatibility. List looks ok as far as it can be serialized without issues.

@avishnyakov avishnyakov added this to the Feb, 2018 - Tech debt #1, Nintex/Docs milestone Jan 31, 2018
@SubPointSupport SubPointSupport self-assigned this May 15, 2018
@SubPointSupport SubPointSupport modified the milestones: June debt #1, Nintex/Docs, 1.2.150-beta1 May 15, 2018
@SubPointSupport
Copy link
Contributor

@vladivanovrf @andreasblueher getting back on this feature. There are a few related tickets and PR:

Here is how it all works:

  1. @avishnyakov made a wrong review on the original ticket suggesting that it is not possible to setup more than one value for FieldDefinition.AddFieldOptions property:

#1073 (comment)

No, @vladivanovrf, unfortunately we can't. This is a "bug", more of a misdesign, overlooked decision made long time ago. We had a similar problem with ListViewDefinition, solved with a new property.

  1. We never looked into more details assuming that FieldDefinition.AddFieldOptions property is a string. Hence, the issue of setting up several values can only be solved by introducing a new property.

  2. Actually, this particular property is enum ( SPMeta2.Enumerations.BuiltInAddFieldOptions) which makes it possible to use normal "&" or "|" operation to combine several bits. This is, surely, absurdly overlooked ticket and whole situation around all this.

Here is how it can be done, and below some tests:

var fieldDef = ModelGeneratorService.GetRandomDefinition<FieldDefinition>(def =>
            {
                def.Hidden = false;

                def.ShowInDisplayForm = true;
                def.ShowInEditForm = true;
                def.ShowInListSettings = true;
                def.ShowInNewForm = true;
                def.ShowInViewForms = true;

                def.AddFieldOptions = BuiltInAddFieldOptions.AddToAllContentTypes
                    | BuiltInAddFieldOptions.AddFieldInternalNameHint;
            });

Full test:

def.AddFieldOptions = BuiltInAddFieldOptions.AddToAllContentTypes

With all this, the whole ticket and related PR done by @andreasblueher aren't necessary.

@vladivanovrf @andreasblueher could you please review this investigation and confirm outcomes?
Again, this is epic and absolutely weirdly overlooked by nearly everyone. Hope that is a good learning so we can improve out attention to details.

@SubPointSupport SubPointSupport modified the milestones: 1.2.150-beta1, 1.2.150-beta2 May 22, 2018
@SubPointSupport
Copy link
Contributor

Folks, @vladivanovrf @andreasblueher, we'd appreciate if you could have a close look on this ticket and related PR #1099

By looking into current functionality, the current ticket and corresponding PR don't have to be moved forward. Looking for your confirmation aiming to close them this week.

@andreasblueher
Copy link
Contributor

Hey guys, I was sick for almost 2 weeks straight and visiting a SharePoint conference the week after so it took some time to get back here. I just tested the possibility to combine BuiltInAddFieldOptions using "|" and i can confirm that it is working as expected. I combined 3 different options and it worked.

If you change those options afterwards it's not updating the field accordingly, but this seems very far fetched to expect this kind of behavior so imho you can close this issue and delete the PR.

@andreasblueher
Copy link
Contributor

I can confirm using "|" to combine those options works fine.

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

No branches or pull requests

4 participants