Campaign lock and unlock#499
Conversation
|
@stevejgordon since the home page has been modified to display a list of current/upcoming campaigns in #417, the Home/Index action will need to be modified to ignore locked campaigns. return View(_dataAccess.Campaigns.Where(c => c.EndDateTime.UtcDateTime.Date > DateTime.UtcNow.Date).ToViewModel().OrderBy(vm => vm.EndDate).ToList()); |
|
@mmoore99 Thanks Mike. Good catch. |
|
@MisterJames can someone look over this PR before I merge? |
| <a asp-controller="Campaign" asp-action="Delete" asp-route-area="Admin" asp-route-id="@Model.Id" class="btn btn-default inline-form"><i class="glyphicon glyphicon-trash"></i></a> | ||
| @if (User.IsUserType(UserType.SiteAdmin)) | ||
| { | ||
| <form asp-controller="Campaign" asp-route-area="Admin" asp-action="LockUnlock" class="inline-form"> |
There was a problem hiding this comment.
Why does this have to be a form? Seems like a lot of extra code. You are already validating that the logged-in user is a SiteAdmin in CampaignController.LockUnlock
There was a problem hiding this comment.
On this one it's not presenting any other page or input via a http get so I felt it best practice to make this require a http post to the action via a form. If we make it just a hyperlink we potentially should add a page to confirm that we want to lock or unlock the campaign which felt unnecessary.
|
Couple other issues:
|
|
@mheggeseth Thanks for your feedback. I'll get the changes made ASAP. |
|
@mheggeseth I worked on this before work this morning and have most of it ready. I want to do a little more before updating the PR. Did you have any thoughts on what we should display in the UI to explain the locked status? |
|
I was thinking a simple tooltip something like "This campaign has been locked by a site administrator. Locked campaigns are not visible to the public or to volunteers." Also, just reading this phrasing makes me think a site admin should enter a comment/reason for locking the campaign. Otherwise, the org admin is in the dark and doesn't know if/what they should change about the campaign. This could be a separate PR. |
stevejgordon
force-pushed
the
feature/#39-CampaignLockUnlock
branch
from
65c2819 to
8cd0c56
Compare
|
@mheggeseth I've pushed an update which address the above feedback. I've left the form for now unless you think we should just use a hyperlink and post action? I plan to open a new issue later for your thoughts on a lock comment etc. |
| .Include(a => a.Tasks) | ||
| .Include(a => a.RequiredSkills) | ||
| .Include(a => a.UsersSignedUp) | ||
| .Where(a => !a.Campaign.Locked) |
There was a problem hiding this comment.
I realize this only has one referrer in ActivityApiController but it doesn't feel quite right to exclude locked activities before returning from the repository class. I'd rather see those locked activities filtered out in ActivityApiController.
|
I had one comment on some code that you added to |
|
@mheggeseth Thanks again for the feedback. Good point and I've updated the PR. |
|
Looks good. |
4 participants
Fixes #39
I've taken the approach to add a bool to the campaign model to mark it as locked.
SiteAdmins can lock/unlock any campaign from the details page using the button I've added.
OrgAdmins will see a message stating the campaign has been locked.
In the admin campaign lister I have added a warning icon to locked campaigns
In the public lister, locked campaigns are not shown
API updated to also not display locked campaigns
Hopefully that makes sense and meets the requirement?