-
Notifications
You must be signed in to change notification settings - Fork 11
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
Isssues/table renaming #6
base: master
Are you sure you want to change the base?
Isssues/table renaming #6
Conversation
…sues/table-renaming
previously manually installed features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic!!! Thank you @hismightiness!
@hismightiness great contribution and thanks for implementing so many best practices in this PR. Can you resolve the one conflict in |
There wasn't any conflicts when I submitted the pull request. It looks like this one has been created by @mathisjay due to a direct commit to |
@david-poindexter I helped @mathisjay revert the commits in |
@david-poindexter Don't merge this yet. I think we may have found a necessary update. |
Okay... We should be good again, David. 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@hismightiness - can you please correct your first comment to the correct GitHub syntax for referencing and autoclosing the issue? See THIS LINK for more information on the correct syntax. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hismightiness
Thanks so much for the excellent additions. Great application of DNN standards and overall code improvements.
I was able to pull the changes and compile successfully. However I ran into some runtime issues.
Here is what I think may be the issues:
-
It appears you may copy/pasted the DTO classes into Info classes for the DAL2. That is fine, but it brought to light a field that was removed from the database table. "activity" does not exist any longer on the actual Visits database table. I think this was working with the LinqToSql version because I never actually tried to reference that field in the LinqToSql object. But in the DAL2 model, I think DAL2 actually expects that field to be there and it isn't, so this throws an runtime error. I noticed that there are a couple of places where "activity" is reference in the context of the DTO, and these should be removed as it is not used anywhere.
-
On the Visitor DTO class, there are two properties 'user_username' and 'user_displayname'. These are set right before sending the DTO over the wire to visitor.User.UserName and visitor.User.DisplayName respectively as a convenience. They don't actually exist on the Visitor table. But it looks like you added them to the VisitorInfo class. I think this is causing DAL2 to think they should actually be on the table.
I was able to get past these issue by commenting out the relevant fields mentioned above.
- The JSON returned from the WebAPIs now all seem to have some weird "k__BackingField" thing attached to every property. as in:
<user_count>k__BackingField
Googling suggest that is caused by adding the [Serializable] attribute to the DTO classes.
https://stackoverflow.com/questions/13022198/how-to-remove-k-backingfield-from-json-when-deserialize
When I comment out the [Serializable] attribute, everything seems to run OK. I'm not sure what that does for anything, but it doesn't appear to be necessary.
This renames the tables, table objects, as well as adds support for DNN's DAL2 so it can be installed in any DNN instance
Resolves issue #5