search

SubPointSolutions / spmeta2

SharePoint artifact provision for .NET platform. Supports SharePoint Online, SharePoint 2019, 2016 and 2013 via CSOM/SSOM.
http://subpointsolutions.com/spmeta2
133 stars 56 forks source link

Added option for AddFieldOptionList #1099

Closed andreasblueher closed 5 years ago

andreasblueher commented 6 years ago

To be able to set multiple AddFieldOptions at once for a field #1091

Neither validator nor generator have been implemented, need more feedback

I would like to continue with this PR before you merge it, as generator and validator don't check any of the options yet and therefor people having issues with those is understandable.

In my opinion you need to check each and every option differently as they have different results. AddFieldinternalNameHint for example is related to the assertion of InternalName if the field is added to a list. Other options are related to other properties and therefor I find it hard to find a starting point. Maybe you have some guidance for me.

andreasblueher commented 6 years ago

Hey guys, could you give me some input on the questions regarding this PR? Not sure which way is best.

SubPointSupport commented 6 years ago

All looks good.

Not sure about the name for this property, AddFieldOptionList might be ok. Let us check a few things.

Testing is not trivial here, indeed. We would have done a few most common cases to see if all works well. For example, if two or more options can be set, and then if the result works as expected. Testing each and every option here would be too hard and rather pointless. Let's focus on most important and frequent ones, and then add later as per user feedback.

andreasblueher commented 6 years ago

My list for most important ones would be:

SubPointSupport commented 6 years ago

We are on the same page here, would have used the same three options. The rest can wait for a while.

As for testing, we should already be testing these for the current AddFieldOptions property. Might be a starting point but let us know if we can help with the implementation.

andreasblueher commented 6 years ago

Great!

I just found the test cases and already wrote one myself, but when I went ahead to check the validators I found that no validation takes places. I'm not sure as I still don't have the full picture regarding the automatic testing, but tests can't be fully working, as long validation is not implemented right?

I search "AddFieldInternalNameHint" and it's used twice in the solution. Once in the BuiltInAddFieldOptions enum and the second time in the ContentTypeScenariosTest class, but nowhere else.

I implemented AddFieldOptionList in a way that it simply extends the default AddFieldOptions property. Doesn't this mean that after implementing the missing tests for BuiltInAddFieldOptions everything should be in place?

andreasblueher commented 6 years ago

Still struggeling to mix Validators and RegressionTests. Do they work separately or do both need to do their part? This test for example does it really (don't want to sound harsh) test if the field is added to the DefaultView? Or this testcase how does it ensure InternalFieldName is set correctly without it being implemented in the validators?

Update: Just found this TODO in the code which in my opinion is related to this and indicates that indeed some more code is missing.

I just remebered a message Anton send me regarding how tests, generators and validations work. I'll read it again!

andreasblueher commented 6 years ago

hey @SubPointSupport I just went back to this PR and I was wondering if you could look through my replys from March 27th and reply to them? Looking through Antons message again didn't help to clear up everything.

SubPointSupport commented 6 years ago

We would like to rework AddFieldOptionList value. Instead or arrray, will switch to int AddFieldOptionValue mapping it back to native SharePoint's AddFieldOptions enum.

Providing raw int value would enable people to put together & or | logic and form the right value as they need to.

Regression testing noted can be found below:

This test for example does it really (don't want to sound harsh) test if the field is added to the DefaultView?

Yes, it does. While one cannot see validation, all magic happen internally further while calling TestModels() method. What happens next:

Every definition has a corresponding validator in CSOM/SSOM. For field, here we go:

Both validator check if AddToDefaultView is set, and then lookup default view (assuming field was added to the list via field link and host is list view / list), and then check if this property is valid by checking if field is present in the default view.

That is more or less logic behind every validator. Deploy the model, and then check every definition's property to see if the SharePoint artifact actually has the right value or right logic.

Or this testcase how does it ensure InternalFieldName is set correctly without it being implemented in the validators?

This test checks is content type ordering can be changed at a content type within a list. The focus of this test is on UniqueContentTypeFieldsOrderDefinition validity, not the fields. This code goes back to a PR in 2015 - https://github.com/SubPointSolutions/spmeta2/commit/6110de9c98aaa6588910a6a9f11b6fe7fa7b9986 and issue https://github.com/SubPointSolutions/spmeta2/issues/742

Hope that help a bit.