Move video url validation to PostEditorService and invalidate containing form if validation fails. #1508

brymut · 2020-04-20T20:45:22Z

This pull request makes the following changes:

move video url validation to PostEditService.validateVideoUrl
use PostEditService.validateVideoUrl to validate url in directive and invalidate form scope when validation fails.
[test] add PostEditService.validateVideoUrl tests

Testing checklist:

Go to settings and create a survey with an 'embed video' field.
Fill in a new survey response based on the one created in the previous step, using a random, non-youtube/non-vimeo-url to the video URL field.
Click save.
Error notification appears and post is NOT saved.
Edit the video URL field to include a valid youtube/vimeo URL and click save.
Post saves successfully.

Quick demo

apologies if too blurry, I can try to resubmit another screenshot on request.

I certify that I ran my checklist

Fixes ushahidi/platform#3904 .

Ping @ushahidi/platfrom

ushbot · 2020-04-20T20:45:26Z

Hey @brymut,
thank you for your Pull Request.

It looks like you haven't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

brymut · 2020-04-20T20:47:43Z

[clabot:check]

ushbot · 2020-04-20T20:47:44Z

@rjmackay It looks like @brymut just signed our Contributor License Agreement. 👍

Always at your service,

clabot

brymut · 2020-04-22T13:33:44Z

Hi @Angamanga , I've managed to get a possible solution working and ready for feedback whenever possible. Also, there was some commented out thumbnail code in the post-video directive that I wasn't sure to remove, should I just leave it for now?

Angamanga · 2020-04-22T21:43:04Z

@brymut This looks good and works fine, super-well done 🎉 . You can absolutely delete the commented out code, thank you for noticing.

Another thing that would be nice to have, if you have time, is to display an error-label under the field, in case the url does not pass the validation. We have not had that for videos previously, but now we can actually add it. We do this for other fields, you can have a look here https://github.com/ushahidi/platform-client/blob/develop/app/main/posts/modify/post-value-edit.html#L22 for an example how it is done in other places. If you don't have time its totally fine and I'll add it as a new issue :).

Before I approve, @rowasc could you do a double-check as well? Since its one of the gnarliest parts of our code and I might have missed something. 🙏

brymut · 2020-04-23T07:20:19Z

Thanks for having a look @Angamanga

@brymut This looks good and works fine, super-well done 🎉 . You can absolutely delete the commented out code, thank you for noticing.

I've amended my last commit to delete the commented out code.

Another thing that would be nice to have, if you have time, is to display an error-label under the field, in case the url does not pass the validation. We have not had that for videos previously, but now we can actually add it. We do this for other fields, you can have a look here https://github.com/ushahidi/platform-client/blob/develop/app/main/posts/modify/post-value-edit.html#L22 for an example how it is done in other places. If you don't have time its totally fine and I'll add it as a new issue :).

At the moment, I don't quite have enough time for the remainder of the week, however, if you can track this a new issue could you please tag/ping me and I'll have a try again over the next week.

brymut · 2020-04-28T09:51:34Z

Hi again @Angamanga , just about to continue with adding the error-label you mentioned soon. Should I go on working off this PR or will it be tracked in a separate issue?

Angamanga · 2020-04-30T04:02:27Z

@brymut Sorry for late reply! Yes, please go ahead and add it to this pr since we haven't had time for a second review yet. Thank you so much for working on this 🎉

tuxpiper · 2020-08-17T04:36:20Z

@Angamanga could you please take another quick look and consider for merging again?

brymut · 2020-08-17T06:32:36Z

Hi @tuxpiper, sorry I’ve taken so long on updating this PR as I agreed to. May I have one more week?

tuxpiper · 2020-08-17T08:51:35Z

no problem @brymut , will check back next week. Thank you!

brymut · 2020-08-24T03:54:41Z

Got some progress done with this over the weekend will push by end of day

brymut · 2020-08-24T16:44:43Z

Okay, so I got a little stuck today and changes started to get messy, so I restarted a new solution that I think should be alright without requiring big edits to the directives involved.

…ing form if validation fails. Changes: - move video url validation to PostEditService.validateVideoUrl - use PostEditService.validateVideoUrl to validate url in directive and setValidity of containing form when validation conditions fail. - [test] mock validateVideoUrl & bindAllFunctionsToSelf functions (and fix typo) - [test] add PostEditService.validateVideoUrl tests

tuxpiper · 2020-08-31T08:26:27Z

hi @brymut! is this ready for review?

brymut · 2020-08-31T08:28:23Z

hi @tuxpiper , sorry I should have been more explicit in my previous comment. Yes, please review.

tuxpiper · 2020-08-31T08:29:57Z

np @brymut 👌 thanks!

@Angamanga could you please take a look?

rowasc · 2020-09-08T15:11:29Z

@Angamanga just bumping this in case it got lost 😅 Thanks!

rowasc · 2020-09-15T02:43:13Z

@Angamanga hey just bumping this one (I know you're busy, just making sure it doesn't get lost ) . Thanks !

Angamanga

This works fine now! Thank you @brymut for your contribution! 🎉

Angamanga · 2020-04-22T21:18:48Z

app/main/posts/modify/post-video.directive.js

                }
            } else {
-                urlError(url);
+                $scope.$parent.form.$setValidity('videoUrlValidation', false, PostVideoController)
+                Notify.error('notify.video.incorrect_url', { url: url });


It would be good to also use the validity to show an error-message to the user. We do this for other fields, you can have a look here https://github.com/ushahidi/platform-client/blob/develop/app/main/posts/modify/post-value-edit.html#L22 for an example how it is done for other fields.

Angamanga · 2020-09-15T08:19:10Z

@rowasc Reviewed and approved 👍

rowasc · 2020-09-15T16:47:16Z

@Obadha2 this can be merged and tested in steve as soon as you're ready

AmTryingMyBest · 2020-09-17T15:32:49Z

QA'd, passes. 👍

brymut changed the title ~~move video url validation to PostEditorService and invalidate containing form if validation fails.~~ WIP: move video url validation to PostEditorService and invalidate containing form if validation fails. Apr 20, 2020

This comment has been minimized.

Sign in to view

brymut force-pushed the bugfix/3904-move-video-url-validation branch 2 times, most recently from 4c2c99b to 27cc105 Compare April 22, 2020 12:59

brymut changed the title ~~WIP: move video url validation to PostEditorService and invalidate containing form if validation fails.~~ Move video url validation to PostEditorService and invalidate containing form if validation fails. Apr 22, 2020

brymut marked this pull request as ready for review April 22, 2020 13:29

brymut force-pushed the bugfix/3904-move-video-url-validation branch from 27cc105 to 2e2956d Compare April 23, 2020 07:16

brymut force-pushed the bugfix/3904-move-video-url-validation branch from 2e2956d to 70a89cb Compare August 24, 2020 16:52

brymut added 2 commits August 25, 2020 16:07

Merge branch 'develop' into bugfix/3904-move-video-url-validation

87288a0

Merge branch 'develop' into bugfix/3904-move-video-url-validation

e7f4960

tuxpiper requested a review from Angamanga August 31, 2020 08:29

Merge branch 'develop' into bugfix/3904-move-video-url-validation

5e1a6ae

Angamanga approved these changes Sep 15, 2020

View reviewed changes

AmTryingMyBest merged commit f3693bc into ushahidi:develop Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move video url validation to PostEditorService and invalidate containing form if validation fails. #1508

Move video url validation to PostEditorService and invalidate containing form if validation fails. #1508

brymut commented Apr 20, 2020 •

edited by Angamanga

Loading

ushbot commented Apr 20, 2020

brymut commented Apr 20, 2020

ushbot commented Apr 20, 2020

This comment has been minimized.

brymut commented Apr 22, 2020 •

edited

Loading

Angamanga commented Apr 22, 2020

brymut commented Apr 23, 2020 •

edited

Loading

brymut commented Apr 28, 2020

Angamanga commented Apr 30, 2020

tuxpiper commented Aug 17, 2020

brymut commented Aug 17, 2020

tuxpiper commented Aug 17, 2020

brymut commented Aug 24, 2020

brymut commented Aug 24, 2020

tuxpiper commented Aug 31, 2020

brymut commented Aug 31, 2020

tuxpiper commented Aug 31, 2020

rowasc commented Sep 8, 2020

rowasc commented Sep 15, 2020

Angamanga left a comment

Angamanga Apr 22, 2020

Angamanga commented Sep 15, 2020

rowasc commented Sep 15, 2020

AmTryingMyBest commented Sep 17, 2020

Move video url validation to PostEditorService and invalidate containing form if validation fails. #1508

Move video url validation to PostEditorService and invalidate containing form if validation fails. #1508

Conversation

brymut commented Apr 20, 2020 • edited by Angamanga Loading

ushbot commented Apr 20, 2020

brymut commented Apr 20, 2020

ushbot commented Apr 20, 2020

This comment has been minimized.

brymut commented Apr 22, 2020 • edited Loading

Angamanga commented Apr 22, 2020

brymut commented Apr 23, 2020 • edited Loading

brymut commented Apr 28, 2020

Angamanga commented Apr 30, 2020

tuxpiper commented Aug 17, 2020

brymut commented Aug 17, 2020

tuxpiper commented Aug 17, 2020

brymut commented Aug 24, 2020

brymut commented Aug 24, 2020

tuxpiper commented Aug 31, 2020

brymut commented Aug 31, 2020

tuxpiper commented Aug 31, 2020

rowasc commented Sep 8, 2020

rowasc commented Sep 15, 2020

Angamanga left a comment

Choose a reason for hiding this comment

Angamanga Apr 22, 2020

Choose a reason for hiding this comment

Angamanga commented Sep 15, 2020

rowasc commented Sep 15, 2020

AmTryingMyBest commented Sep 17, 2020

brymut commented Apr 20, 2020 •

edited by Angamanga

Loading

brymut commented Apr 22, 2020 •

edited

Loading

brymut commented Apr 23, 2020 •

edited

Loading