U4-11296 - umbracoConsent comment column should allow nulls

Created by Simon Dingley 01 May 2018, 11:42:38 Updated by Sebastiaan Janssen 20 Jun 2018, 15:10:31

Tags: Up For Grabs PR

At the moment the comment column does not allow nulls yet the default value for the optional comment parameter when calling RegisterConsent on the ConsentService is null resulting in an exception being thrown if no comment parameter is passed.

To reproduce:

var consentSvc = UmbracoContext.Current.Application.Services.ConsentService; consentSvc.RegisterConsent("source", "context", "action", ConsentState.Granted);


Simon Dingley 01 May 2018, 12:42:24

I'll have a bash at this, I've not submitted any PR's before with migrations so will put together a first attempt for feedback.

Sebastiaan Janssen 01 May 2018, 12:50:09

Maybe make it much easier and just pass an empty string instead of a null? I don't know if we really need to update the table, I've also not looked at the code at all, so not sure.

But if needed then it should be a migration yes (make sure to target 7.11.0 for that!). For inspiration you can look at other migrations of course.

Simon Dingley 01 May 2018, 13:00:13

We don't really want to be storing empty strings in the database though do we? I've done it now anyway so take a look when you can and if it's not the desired approach I'll go with your approach.

Sebastiaan Janssen 01 May 2018, 13:27:05

Cool, got a link to a PR?

Simon Dingley 01 May 2018, 13:35:25

Having problems with my Git repo at the moment - https://our.umbraco.org/forum/contributing-to-umbraco-cms//91821-git-line-endings-for-umbracocms-solution

I can submit my PR still but Github says "Can’t automatically merge. Don’t worry, you can still create the pull request", so might create more work for you?

Simon Dingley 01 May 2018, 13:38:20


Priority: Normal

Type: Bug

State: Fixed


Difficulty: Normal


Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.10.4

Due in version: 7.12.0


Story Points: