-
Notifications
You must be signed in to change notification settings - Fork 622
Campaign lock and unlock #499
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
Changes from all commits
071d8cc
657ad35
29dfb6c
cb803ba
5c97feb
8cd0c56
d421798
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| using AllReady.Areas.Admin.Features.Campaigns; | ||
| using System.Linq; | ||
| using Xunit; | ||
|
|
||
| namespace AllReady.UnitTest.Areas.Admin.Features.Campaigns | ||
| { | ||
| public class LockUnlockCampaignCommandHandlerTests : InMemoryContextTest | ||
| { | ||
| [Fact] | ||
| public void LockedCampaignIsUnlocked() | ||
| { | ||
| // Arrange | ||
| var handler = new LockUnlockCampaignCommandHandler(Context); | ||
|
|
||
| // Act | ||
| var campaign = Context.Campaigns.FirstOrDefault(c => c.Name == "Locked Campaign"); | ||
| handler.Handle(new LockUnlockCampaignCommand { CampaignId = campaign.Id }); | ||
| var result = Context.Campaigns.FirstOrDefault(c => c.Name == "Locked Campaign"); | ||
|
|
||
| // Assert | ||
| Assert.False(result.Locked); // Campaign should now be unlocked | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UnlockedCampaignIsLocked() | ||
| { | ||
| // Arrange | ||
| var handler = new LockUnlockCampaignCommandHandler(Context); | ||
|
|
||
| // Act | ||
| var campaign = Context.Campaigns.FirstOrDefault(c => c.Name == "Unlocked Campaign"); | ||
| handler.Handle(new LockUnlockCampaignCommand { CampaignId = campaign.Id }); | ||
| var result = Context.Campaigns.FirstOrDefault(c => c.Name == "Unlocked Campaign"); | ||
|
|
||
| // Assert | ||
| Assert.True(result.Locked); // Campaign should now be locked | ||
| } | ||
|
|
||
| protected override void LoadTestData() | ||
| { | ||
| CampaignsHandlerTestHelper.LoadCampaignssHandlerTestData(Context); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| using MediatR; | ||
|
|
||
| namespace AllReady.Areas.Admin.Features.Campaigns | ||
| { | ||
| public class LockUnlockCampaignCommand : IRequest | ||
| { | ||
| public int CampaignId {get; set;} | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| using AllReady.Models; | ||
| using MediatR; | ||
| using System.Linq; | ||
|
|
||
| namespace AllReady.Areas.Admin.Features.Campaigns | ||
| { | ||
| public class LockUnlockCampaignCommandHandler : RequestHandler<LockUnlockCampaignCommand> | ||
| { | ||
| private AllReadyContext _context; | ||
|
|
||
| public LockUnlockCampaignCommandHandler(AllReadyContext context) | ||
| { | ||
| _context = context; | ||
|
|
||
| } | ||
| protected override void HandleCore(LockUnlockCampaignCommand message) | ||
| { | ||
| var campaign = | ||
| _context.Campaigns.SingleOrDefault(c => c.Id == message.CampaignId); | ||
|
|
||
| if (campaign != null) | ||
| { | ||
| campaign.Locked = !campaign.Locked; | ||
|
|
||
| _context.Update(campaign); | ||
| _context.SaveChanges(); | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,6 @@ | |
| using Microsoft.AspNet.Mvc; | ||
| using AllReady.Models; | ||
| using AllReady.ViewModels; | ||
| using Microsoft.Data.Entity; | ||
|
|
||
| namespace AllReady.Controllers | ||
| { | ||
|
|
@@ -21,7 +20,7 @@ public CampaignController(IAllReadyDataAccess dataAccess) | |
| [Route("~/[controller]")] | ||
| public IActionResult Index() | ||
| { | ||
| return View(_dataAccess.Campaigns.ToViewModel().ToList()); | ||
| return View(_dataAccess.Campaigns.Where(c => !c.Locked).ToViewModel().ToList()); | ||
| } | ||
|
|
||
| [HttpGet] | ||
|
|
@@ -30,8 +29,8 @@ public IActionResult Details(int id) | |
| { | ||
| var campaign = _dataAccess.GetCampaign(id); | ||
|
|
||
| if (campaign == null) | ||
| HttpNotFound(); | ||
| if (campaign == null || campaign.Locked) | ||
| return HttpNotFound(); | ||
|
|
||
| return View("Details", new CampaignViewModel(campaign)); | ||
| } | ||
|
|
@@ -43,7 +42,7 @@ public IActionResult LocationMap(int id) | |
| var campaign = _dataAccess.GetCampaign(id); | ||
|
|
||
| if (campaign == null) | ||
| HttpNotFound(); | ||
| return HttpNotFound(); | ||
|
|
||
| return View("Map", new CampaignViewModel(campaign)); | ||
| } | ||
|
|
@@ -53,19 +52,22 @@ public IActionResult LocationMap(int id) | |
| public IEnumerable<CampaignViewModel> Get() | ||
| { | ||
| return _dataAccess.Campaigns | ||
| .Where(c => !c.Locked) | ||
| .Select(x => new CampaignViewModel(x)); | ||
| } | ||
|
|
||
| // GET api/values/5 | ||
| [HttpGet("{id}")] | ||
| public CampaignViewModel Get(int id) | ||
| public ActionResult Get(int id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this used? I can't figure it out.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is something I noticed when testing the API. Previously there was a HttpNotFound(); line which seems to do nothing since it wasn't being actually returned. However to return it the controller needs to be an action result rather than just the model. I may have misunderstood something here about how the MVC Core controllers work for API responses though? If this is truly an issue we may need to check the other API endpoints as well. |
||
| { | ||
| var campaign = _dataAccess.GetCampaign(id); | ||
|
|
||
| if (campaign == null) | ||
| HttpNotFound(); | ||
| if (campaign == null || campaign.Locked) | ||
| { | ||
| return HttpNotFound(); | ||
| } | ||
|
|
||
| return campaign.ToViewModel(); | ||
| return Json(campaign.ToViewModel()); | ||
| } | ||
| } | ||
| } | ||
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.
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.LockUnlockThere 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.
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.