Bombs on activity image upload if no file selected

[DevUnit1]

On the Admin Edit Activity screen, if I load an activity with no image file and click the Upload button, the application throws a null reference exception. It should instead ignore or not allow an image upload with no file selected.

[simon-20]

For me this is breaking whether or not a file is selected. I think it's because there is no Route attribute specified, but there are also some other issues, like typos in some of the IDs in the <form> element.

I'll take a look at this, unless it isn't needed for the November MVP and other things should be prioritized?

[simon-20]

This upload form is using RedirectToRoute (to the Activity edit page) on both success and failure of image upload. Ignoring a couple of typos which are currently breaking the form (it doesn't use Html.BeginForm but is hand generated), this means that ModelState will be lost upon failure, such that no error appears. In actual fact, because the Edit page is using the Activity class as its model, and not it's own ViewModel, the <input type='file'> upload element isn't part of the model anyway, so no validation would be happening in any case.

Rather than use TempData to persist validation messages concerning the <input type='file'> element back to the Edit action, it seems like this would be an opportune moment to alter the Create and Edit activity pages (both of which use the Edit.cshtml view) to have their own ViewModel.

This will allow the <input type='file'> element to be added to the model and will then allow full flexibility for validation (e.g. we could (in due course) derive from RequiredAttrbute and check it is indeed an image file that is uploaded).

Let me know if you think this is the wrong approach; if it sounds sensible I'll start working on it.

[simon-20]

I think I have this basically working now, having converted the create/edit/activity image upload page to use two different ViewModels rather than the simply the Activity object as its model. This enables each of the forms to be separately validated.

It's successfully uploading images to Azure.

I'll test everything more thoroughly tomorrow morning and push it for review.

[simon-20]

This problem also exists for the Campaign Admin Edit page: it too crashes when no file is selected.

The issue is as described above.

@dpaquette, what would you suggest as the best way for fixing this? If your refactorings of all the admin pages is now complete, the solution I committed last week could perhaps be reimplemented on top of those modifications (i.e., we could have two models, one for each form, which would allow validation on the image form).

Alternatively, the PostActivityFile and PostCampaignFile actions could be modified to check for a null value, and then redirect back to the appropriate edit form. The redirection would need to include something to indicate the error so that a message could be displayed. The latter would certainly be a simpler solution - would it be too hacky, putting something on the query string (say) to indicate the error?

[dpaquette]

Touch base with @joelhulen. I think he was doing something related to images. Just want to make sure you 2 are not overlapping on tasks.

[joelhulen]

Yes, that's true. I wasn't finished with the changes since I was rushing to
get it up for testing toward the end of the hackathon. Feel free to make
any necessary adjustments, however.

On Monday, November 9, 2015, David Paquette notifications@github.com
wrote:

Touch base with @joelhulen https://github.com/joelhulen. I think he was
doing something related to images. Just want to make sure you 2 are not
overlapping on tasks.

—
Reply to this email directly or view it on GitHub
#48 (comment).

[simon-20]

Ah okay, I'll leave it for now if you're doing stuff on this @joelhulen. I hope my PR #301 didn't overlap with anything image-related you were doing.

My main question for the above, which maybe of interest for whoever ends up working on it, concerns the design decision of whether to split the viewmodel so that the file upload form can be validated in the normal way, or whether to simply do the error checking on the post action and then redirect to the original edit page indicating some kind of error via the querystring.

[simon-20]

@joelhulen are your changes with respect to images complete now? Shall I go ahead and fix up the activity edit page in a manner similar to how you simplified the campaign one, or are you handling this one too?

[joelhulen]

@ksk100 yes, my changes with respect to images on the campaign edit page are complete. Please go ahead and follow a similar pattern for the activity edit page. I wouldn't be able to get to it until late tonight.

One thing to look for is the issue you brought up about the image being required. If you fix that for activities, please also apply the fix to campaigns.

[simon-20]

Okay no problem.
On 12 Nov 2015 17:27, "Joel Hulen" notifications@github.com wrote:

@ksk100 https://github.com/ksk100 yes, my changes with respect to
images on the campaign edit page are complete. Please go ahead and follow a
similar pattern for the activity edit page. I wouldn't be able to get to it
until late tonight.

One thing to look for is the issue you brought up about the image being
required. If you fix that for activities, please also apply the fix to
campaigns.

—
Reply to this email directly or view it on GitHub
#48 (comment).

[simon-20]

One question: as it stands the Create Activity page doesn't show the image upload form; that only becomes accessible via the Edit page once the activity has been created. Was there a reason for that behaviour and should it be retained?

(Incidentally, as it's already getting late here I probably won't get this finished and tested tonight; but it'll be finished off first thing tomorrow, with the fix for the campaign page too; if that will be too late do take over).

[joelhulen]

I don't know why it was set up that way, to be honest. If you mimic the image behavior I've set up in the Campaign edit form, then you'll be able to upload the image on create as well as edit.

[simon-20]

Okay will do.
On 12 Nov 2015 18:38, "Joel Hulen" notifications@github.com wrote:

I don't know why it was set up that way, to be honest. If you mimic the
image behavior I've set up in the Campaign edit form, then you'll be able
to upload the image on create as well as edit.

—
Reply to this email directly or view it on GitHub
#48 (comment).