ProfilesComponent: Add field validation for custom profiles #623

dingyuchen · 2021-03-04T11:26:54Z

Summary

Fixes #567

The app proceeds silently if the selected profiles.json does not have a profileName or encodedText field.
(encodedText refers to the repo holding the settings.json / data.csv files)

Proposed Commit Message

ProfilesComponent: Add field validation

The app proceeds silently if the selected profiles.json does not have a profileName or encodedText field.

Let's:
* Add generic function to validate existence of keys in an object
* Validate profiles during upload and selection of profiles
* Open an error dialog if the profile object is malformed

seanlowjk

Just a few comments from my side! Please have a look at the bug where the None option is slected!

src/app/shared/lib/validate.ts

src/app/auth/profiles/profiles.component.ts

seanlowjk

Just a single comment here!

Other than that, thanks for the good work! 💯

src/app/auth/profiles/json-parse-error-dialog/json-parse-error-dialog.component.html

Co-authored-by: Low Jun Kai, Sean <42912708+seanlowjk@users.noreply.github.com>

codecov-io · 2021-03-07T02:06:55Z

Codecov Report

Merging #623 (e014860) into master (ddb57c2) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
+ Coverage   67.71%   67.80%   +0.08%     
==========================================
  Files          73       75       +2     
  Lines        2187     2196       +9     
  Branches      201      202       +1     
==========================================
+ Hits         1481     1489       +8     
  Misses        664      664              
- Partials       42       43       +1

Impacted Files	Coverage Δ
src/environments/environment.gen.ts	`80.00% <ø> (ø)`
src/app/core/models/profile.model.ts	`100.00% <100.00%> (ø)`
src/app/shared/lib/validate.ts	`100.00% <100.00%> (ø)`
src/app/shared/issue-tables/issue-sorter.ts	`44.00% <0.00%> (-4.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddb57c2...e014860. Read the comment docs.

anubh-v

The PR is on the right track, but a bit of refactoring can be done to make this more testable.

Let's :

Move the Profile interface, as well as the profileSchema into a new profile.model.tsmodule
Export a isValidProfile method from this module. This method will use the schema internally
Use isValidProfile in the ProfilesComponent
Add some tests for isValidProfile

src/app/auth/profiles/profiles.component.ts

…ATcher into assert-profiles.json-fields

ptvrajsk

Great job with this! 👍 I like how you used the Schema interface in creating a clean way to validate our profiles.json. Just some minor discussion points.

ptvrajsk · 2021-03-10T05:29:52Z

src/app/core/models/profile.model.ts

+
+const profileSchema: Schema = {
+  profileName: { required: true, validate: (value) => !!value },
+  encodedText: { required: true, validate: (value) => !!value }


Should we have the extra layer of validation to make sure that encoded text follows the org/repo format? So the validate function can use regex or any form of string manipulation to verify the encoded text adheres to the right format?

Yup that's a good point. i guess we'll be matching for string/string?

anubh-v

Looks good. @CATcher-org/2021-devs any further comments?

src/app/core/models/profile.model.ts

Co-authored-by: Anubhav <35621759+anubh-v@users.noreply.github.com>

ptvrajsk

LGTM 👍

dingyuchen added 7 commits February 20, 2021 12:21

Refactor filters

5eea018

Refactor filters

8c0cf59

Merge branch 'master' of github.com:dingyuchen/CATcher

e37ee23

Merge branch 'master' of github.com:CATcher-org/CATcher

8af0716

Merge branch 'master' of github.com:CATcher-org/CATcher

85f5769

Validate profiles.json fields on upload

11edfcd

Add validation in profile selection

482e6bd

dingyuchen requested a review from a team March 4, 2021 11:28

dingyuchen added the s.ToReview label Mar 4, 2021

Fix comments in object validation

25e86c9

seanlowjk reviewed Mar 5, 2021

View reviewed changes

src/app/shared/lib/validate.ts Outdated Show resolved Hide resolved

src/app/auth/profiles/profiles.component.ts Show resolved Hide resolved

dingyuchen and others added 4 commits March 5, 2021 17:12

Merge branch 'master' into assert-profiles.json-fields

22df0c7

Add exception for blank profile and add more type declarations

7a04b2a

Fix lint error

09615dd

Merge branch 'master' into assert-profiles.json-fields

289ffed

seanlowjk reviewed Mar 6, 2021

View reviewed changes

src/app/auth/profiles/json-parse-error-dialog/json-parse-error-dialog.component.html Outdated Show resolved Hide resolved

dingyuchen and others added 2 commits March 7, 2021 10:04

Fix error message

2006ce3

Co-authored-by: Low Jun Kai, Sean <42912708+seanlowjk@users.noreply.github.com>

Merge branch 'master' into assert-profiles.json-fields

2d7c7f0

Merge branch 'master' into assert-profiles.json-fields

17fc776

dingyuchen requested a review from anubh-v March 8, 2021 05:44

anubh-v reviewed Mar 8, 2021

View reviewed changes

src/app/auth/profiles/profiles.component.ts Outdated Show resolved Hide resolved

dingyuchen added 8 commits March 9, 2021 10:56

Refactor profile validation

0c856cc

Undo style changes

24eafd3

Add missing newline

e1dafce

Fix relative pathing

235b68c

Fix pathing in environment.gen.ts

3746714

Add tests for isValidProfile

36eac8d

Fix lint error

d40be5e

Merge branch 'master' into assert-profiles.json-fields

9d53ac9

dingyuchen added 4 commits March 9, 2021 11:28

Undo style changes

898b478

Merge branch 'assert-profiles.json-fields' of github.com:dingyuchen/C…

c5c3fa1

…ATcher into assert-profiles.json-fields

Undo style changes

4f636e5

Fix failing tests

6993f5a

ptvrajsk reviewed Mar 10, 2021

View reviewed changes

dingyuchen added 2 commits March 10, 2021 15:36

Validate encodedText field to be the form 'org/repo'

8fd452b

Merge branch 'master' into assert-profiles.json-fields

2452a2a

anubh-v approved these changes Mar 11, 2021

View reviewed changes

src/app/core/models/profile.model.ts Outdated Show resolved Hide resolved

Add comment to explain validation regex

e014860

Co-authored-by: Anubhav <35621759+anubh-v@users.noreply.github.com>

ptvrajsk self-requested a review March 12, 2021 05:00

ptvrajsk approved these changes Mar 12, 2021

View reviewed changes

anubh-v added s.ToMerge and removed s.ToReview labels Mar 12, 2021

anubh-v merged commit 1f9f945 into CATcher-org:master Mar 12, 2021

anubh-v changed the title ~~Assert 'profiles.json' to have required fields~~ ProfilesComponent: Add field validation for custom profiles Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProfilesComponent: Add field validation for custom profiles #623

ProfilesComponent: Add field validation for custom profiles #623

dingyuchen commented Mar 4, 2021 •

edited by anubh-v

seanlowjk left a comment

seanlowjk left a comment

codecov-io commented Mar 7, 2021 •

edited

anubh-v left a comment •

edited

ptvrajsk left a comment

ptvrajsk Mar 10, 2021 •

edited

dingyuchen Mar 10, 2021

ptvrajsk Mar 10, 2021

anubh-v left a comment

ptvrajsk left a comment

ProfilesComponent: Add field validation for custom profiles #623

ProfilesComponent: Add field validation for custom profiles #623

Conversation

dingyuchen commented Mar 4, 2021 • edited by anubh-v

Summary

Proposed Commit Message

seanlowjk left a comment

Choose a reason for hiding this comment

seanlowjk left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 7, 2021 • edited

Codecov Report

anubh-v left a comment • edited

Choose a reason for hiding this comment

ptvrajsk left a comment

Choose a reason for hiding this comment

ptvrajsk Mar 10, 2021 • edited

Choose a reason for hiding this comment

dingyuchen Mar 10, 2021

Choose a reason for hiding this comment

ptvrajsk Mar 10, 2021

Choose a reason for hiding this comment

anubh-v left a comment

Choose a reason for hiding this comment

ptvrajsk left a comment

Choose a reason for hiding this comment

dingyuchen commented Mar 4, 2021 •

edited by anubh-v

codecov-io commented Mar 7, 2021 •

edited

anubh-v left a comment •

edited

ptvrajsk Mar 10, 2021 •

edited