-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enforce unique Rules (Old Urls) #62
Conversation
} | ||
catch | ||
{ | ||
return false; |
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.
I feel that at least what we should do here is to log error through ILogger - I know errors are ignored in every other method (ie Delete, ClearAll) but maybe we should fix that? What do you think @m-jedynak
(Even better solution is to return errror message to end user)
Delete(duplicate.Id.ExternalId); | ||
} | ||
} | ||
return duplicates; |
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.
Since this method returns IQueryable<RedirectRule>
(which I'm not 100% sure is required, since it is not used anywhere) I think to be on safe side we should check for null here so the last line:
return duplicates ?? Enumerable.Empty<RedirectRule>().AsQueryable();
so we do not hit NullReferenceException by accident
: _redirectRuleRepository.Delete(id); | ||
bool deletedSuccessfully; | ||
|
||
if (id == _clearAllGuid) { |
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.
I'm not fluent with EpiServer store concept but I wonder if this is possible to have separate methods in controller (I believe it needs to be supported by JS side of EpiServer store): Delete(GuidId), ClearAll() and RemoveAllDuplicates() ?
We assume that duplicate is a redirect rule with the same oldUrl unless this rule RedirectRuleType is set to Regex.
Update behavior
New feature