#180 added property blacklist to Input.Model

[mattcowen]

You can now ignore properties on a viewmodel using lambda expressions via a params on the Input.Model method of PageWriter.

[robdmoore]

This looks good, I wonder if we can get a test to cover the new functionality. Do you feel comfortable adding one? I can help out if not.

[mattcowen]

I modified an existing test which uses MvcMusicStore.FunctionalTests/Step3/Pages/RegisterPage.cs. Happy to do another if necessary though.

[mattcowen]

I've found a problem with this code. If you have a value type it is getting boxed in the cast to object which means the Body of the Expression can not be cast to MemberExpression. Not sure why I didn't see this with the boolean property in my test. Looking into a solution but any thoughts welcome.

[mwhelan]

Hey @mattcowen . Just to mention, it's also not compiling on the TeamCity server, nor in your github repo. It's missing a reference to Mvc3. I've actually changed the reference to a NuGet package in my PR, but that's not been accepted yet either. You can see the build server here: http://teamcity.ginnivan.net/project.html?projectId=TestStack_Seleno&tab=projectOverview (login as guest)

[mattcowen]

Ok, I don't understand why that is not building. I can see the reference to MVC 3 in both sample projects and it builds locally too of course. Any ideas on what I need to do?

[mwhelan]

Yes, having a look at your TestStack.Seleno.csproj file, I notice that it has this Private tag set to true.

`

True

</Reference>`

I've not come across that before, but adding MVC3 on my dev machine I get this in the tag, which sorts the problem (note the version is also slightly different):

<Reference Include="System.Web.Mvc, Version=3.0.0.1, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />

Perhaps the simplest solution right now is to just delete the MVC reference and re-add it in each project that has MVC3, and verify that you get the correct tag in each .csproj file.

[mattcowen]

I've removed the private tag but I really don't understand how that is the problem. The history on that project file shows it was set to true before.

[mwhelan]

I'm not sure either. It just seemed a bit strange, but if it was already there then it can't have been the issue. I'm hoping to switch over to the NuGet package soon, in which case it becomes moot. I'm having some build server issues though, so it might be a few days before we can sort that.

[robdmoore]

Private just means don't copy to bin directory when building: http://msdn.microsoft.com/en-us/library/bb629388(v=vs.110).aspx

It's an existing problem caused when you don't have mvc3 in the GAC.

Ignore it since it will be fixed when we merge michaels PR

On 23 Oct 2014, at 6:42 pm, Michael Whelan notifications@github.com wrote:

I'm not sure either. It just seemed a bit strange, but if it was already there then it can't have been the issue. I'm hoping to switch over to the NuGet package soon, in which case it becomes moot. I'm having some build server issues though, so it might be a few days before we can sort that.

— Reply to this email directly or view it on GitHub.

[robdmoore]

This explains why it's started failing: http://bit.ly/1qJ7Hpb

I'm not sure either. It just seemed a bit strange, but if it was already there then it can't have been the issue. I'm hoping to switch over to the NuGet package soon, in which case it becomes moot. I'm having some build server issues though, so it might be a few days before we can sort that.

— Reply to this email directly or view it on GitHub.

[mattcowen]

Well, I'm a bit stuck. I have the following which solves the problem from a conceptual level but not quite sure of the best way to apply something like this to that PageWriter class. Any thoughts?

void Main()
{

    var x = new Writer<ViewModel>();
    Debug.WriteLine(x.GetInfo<int>(t => t.Age).Name);
    Debug.WriteLine(x.GetInfo<bool>(t => t.IsEnabled).Name);
}
public class ViewModel 
{
    public string Name { get; set; }
    public int Age { get; set; }
    public bool IsEnabled { get; set; }
    public string UserId { get; set; }
}

public class Writer<T> where T : class, new()
{

    public PropertyInfo GetInfo<U>(Expression<Func<T, U>> propertyBlacklist) 
    {
        var x = propertyBlacklist.Body as MemberExpression;

        return x.Member as PropertyInfo;
    }

}

[mattcowen]

I suppose one fallback is to pass a params of MemberInfo/PropertyInfo instead of lambda expressions.

[mwhelan]

I really like strongly typed parameters, but I think it can get quite unwieldy for multiple parameters. I think that passing the parameters as strings, and throwing a clear exception if one of the strings is not found, is simpler to implement and maintain and achieves type safety, albeit at runtime rather than design time.

[mattcowen]

By the way Rob, that article on the failed build was a good find thanks.

Ok, leave this with me. I will implement something that works as soon as I can.

[mattcowen]

I've submitted my change to make params string[] instead of lambda. I added a helper method to UIComponent to allow you to use lambda expressions to return a string if you want to.

Sorry for the delay

[robdmoore]

This looks good. It's actually conflicted now after a big merge we did (sorry!) if you need help fixing the merge conflicts let us know.

I just added a couple of minor comments - other than that I think this is fine to go in.

Only other thing I'm left wondering is if it's confusing to someone scanning a page object that has the following code:

Input.Model(model, null, x => x.LobValue, x => x.RegistrationCode);

If you don't look at the parameter name it would be reasonable to assume what it's actually doing is including LobValue and RegistrationCode only, rather than the opposite.

Perhaps there is a general concept here we can wrap up like some sort of property inclusion rule or something. e.g.

Input.Model(model, with: Properties<ViewModelType>.Include(m => m.LobValue, m => m.RegistrationCode))
Input.Model(model, with: Properties<ViewModelType>.Exclude(m => m.LobValue, m => m.RegistrationCode))

where the with value is some sort of object like this:

public interface IPropertyInclusionRule
{
    bool Include(string propertyName);
}

Only problem is that's more verbose and you have to specify the type of the view model again. I wonder if there is some sort of middle ground that has the advantage of both? cc @mwhelan @MehdiK

[robdmoore]

Only other thing is what to do about nested properties. Do we care - should it only deal with top level properties (as it does now) or should we deal with nested properties? The Id generator stuff deals with nested properties (at least the Mvc variant of it does) for instance.

[mwhelan]

I'm not sure. What is an example of when you need to input nested properties?

[mwhelan]

Not sure we need Include. I think the assumption is all are included, unless black-listed properties are excluded? I think all the lambda expression arrays are verbose, but I think your suggestion communicates the exclusion a bit more. Maybe it's fine the way it is though. Perhaps if you are needing to exclude properties you can specify "propertyBlacklist" as named parameter and that would make the intent clear - assuming you can do that with params! ;-)

[robdmoore]

Yeah I would have suggested that, but it doesn't work with params; you have to explicitly specify the array.

On 2 Nov 2014, at 5:40 pm, Michael Whelan notifications@github.com wrote:

Not sure we need Include. I think the assumption is all are included, unless black-listed properties are excluded? I think all the lambda expression arrays are verbose, but I think your suggestion communicates the exclusion a bit more. Maybe it's fine the way it is though. Perhaps if you are needing to exclude properties you can specify "propertyBlacklist" as named parameter and that would make the intent clear - assuming you can do that with params! ;-)

— Reply to this email directly or view it on GitHub.

[mattcowen]

Please review my PR. HTH

[robdmoore]

I'm happy to merge this in; I like the change. We will need to document the breaking change in BREAKING_CHANGES.md (talking to fields by name rather than id) though. Are you able to have a go at that as part of this PR - keen not to delay the PR any further though?

FYI - for the future, it would be good if we can have separate PRs for separate changes - in this case there are two changes - property blacklist and changing to name. Just makes it easier to review and merge smaller changesets with a single logical change.

[mattcowen]

No problem, I will update the breaking changes shortly.

Ok will do in future. Makes sense.

Thanks Matt

On Mon, Feb 16, 2015 at 1:32 AM, Rob Moore notifications@github.com wrote:

I'm happy to merge this in; I like the change. We will need to document the breaking change in BREAKING_CHANGES.md (talking to fields by name rather than id) though. Are you able to have a go at that as part of this PR - keen not to delay the PR any further though?

FYI - for the future, it would be good if we can have separate PRs for separate changes - in this case there are two changes - property blacklist and changing to name. Just makes it easier to review and merge smaller changesets with a single logical change.

— Reply to this email directly or view it on GitHub https://github.com/TestStack/TestStack.Seleno/pull/182#issuecomment-74449255 .

[robdmoore]

Nice. Ping back when you've updated breaking changes doc. Just mention the version number in the doc as 0.9.0 - I'll bump it to that

Rob Moore | Readify Principal Consultant, Technical Specialist (Microsoft Azure) | m +61 400 777 763 | e rob.moore@readify.net | w readify.net

On 17 Feb 2015, at 6:10 am, Matt Cowen notifications@github.com wrote:

No problem, I will update the breaking changes shortly.

Ok will do in future. Makes sense.

Thanks Matt

On Mon, Feb 16, 2015 at 1:32 AM, Rob Moore notifications@github.com wrote:

I'm happy to merge this in; I like the change. We will need to document the breaking change in BREAKING_CHANGES.md (talking to fields by name rather than id) though. Are you able to have a go at that as part of this PR - keen not to delay the PR any further though?

FYI - for the future, it would be good if we can have separate PRs for separate changes - in this case there are two changes - property blacklist and changing to name. Just makes it easier to review and merge smaller changesets with a single logical change.

— Reply to this email directly or view it on GitHub https://github.com/TestStack/TestStack.Seleno/pull/182#issuecomment-74449255 .

— Reply to this email directly or view it on GitHub.

[mattcowen]

This has been done

On Mon, Feb 16, 2015 at 10:27 PM, Rob Moore notifications@github.com wrote:

Nice. Ping back when you've updated breaking changes doc. Just mention the version number in the doc as 0.9.0 - I'll bump it to that

Rob Moore | Readify Principal Consultant, Technical Specialist (Microsoft Azure) | m +61 400 777 763 | e rob.moore@readify.net | w readify.net

On 17 Feb 2015, at 6:10 am, Matt Cowen notifications@github.com wrote:

No problem, I will update the breaking changes shortly.

Ok will do in future. Makes sense.

Thanks Matt

On Mon, Feb 16, 2015 at 1:32 AM, Rob Moore notifications@github.com wrote:

I'm happy to merge this in; I like the change. We will need to document the breaking change in BREAKING_CHANGES.md (talking to fields by name rather than id) though. Are you able to have a go at that as part of this PR - keen not to delay the PR any further though?

FYI - for the future, it would be good if we can have separate PRs for separate changes - in this case there are two changes - property blacklist and changing to name. Just makes it easier to review and merge smaller changesets with a single logical change.

— Reply to this email directly or view it on GitHub < https://github.com/TestStack/TestStack.Seleno/pull/182#issuecomment-74449255

.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/TestStack/TestStack.Seleno/pull/182#issuecomment-74582167 .

[robdmoore]

Thanks for the contribution mate!