Skip to content

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Jan 15, 2020

Fixes #6742

Changes proposed in this pull request:

added age_group field in attendee schema and corresponding model.

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@snitin315 snitin315 requested a review from kushthedude January 15, 2020 16:02
@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #6744 into development will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #6744      +/-   ##
==============================================
+ Coverage        65.39%   65.4%   +0.01%     
==============================================
  Files              300     300              
  Lines            15332   15337       +5     
==============================================
+ Hits             10026   10031       +5     
  Misses            5306    5306
Impacted Files Coverage Δ
app/models/custom_form.py 92.68% <ø> (ø) ⬆️
app/api/schema/attendees.py 98.33% <100%> (+0.08%) ⬆️
app/models/ticket_holder.py 79.41% <100%> (+0.3%) ⬆️
app/api/helpers/static.py 100% <100%> (ø) ⬆️

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 0183671...8b657ea. Read the comment docs.

kushthedude
kushthedude previously approved these changes Jan 16, 2020
@iamareebjamal
Copy link
Member

This will accept any string as age group which was never the intention

@snitin315
Copy link
Member Author

snitin315 commented Jan 16, 2020

This will accept any string as age group which was never the intention

@iamareebjamal On the frontend side I have created a dictionary for ageGroups as there is for genders. Hence the user can select only one of the options.

Screenshot from 2020-01-14 21-27-06

@iamareebjamal
Copy link
Member

Doesn't matter really. I can set ooga booga and server will still accept it. Frontend checks are as useless as Windows Defender against viruses

@snitin315
Copy link
Member Author

Doesn't matter really. I can set ooga booga and server will still accept it. Frontend checks are as useless as Windows Defender against viruses

Any suggetions whats should be done?

@iamareebjamal
Copy link
Member

Add server side validation that the age group is correct

'30 to 39',
'40 to 49',
'50 or above'
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no newline at end of file

@snitin315 snitin315 force-pushed the development branch 2 times, most recently from ae7dbff to c99dd86 Compare January 16, 2020 11:32
'40 to 49',
'50 or above'
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line at end of file

@iamareebjamal
Copy link
Member

This should be dynamic but will do it for now

'40 to 49',
'50 or above'
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@iamareebjamal iamareebjamal changed the title add age_group field in ticket holder model feat: add age_group field in ticket holder model Jan 17, 2020
@auto-label auto-label bot added the feature label Jan 17, 2020
Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple Migration Heads

@snitin315 snitin315 force-pushed the development branch 2 times, most recently from 661a9d5 to 6938ec4 Compare January 17, 2020 12:45
'30 to 39',
'40 to 49',
'50 or above'
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
]
]

@niranjan94
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

See the complete overview on Codacy

@codedsun codedsun mentioned this pull request Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ageGroup field in Attendee Model

5 participants