Skip to content
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

fix: added check for required-fields and rendering gender #3567

Merged
merged 1 commit into from
Nov 24, 2019

Conversation

akshat0047
Copy link
Member

Fixes #3406

Short description of what this resolves:

When displaying the attendee list on the frontend, few blank fields with labels were getting rendered I have fixed it by implementing a check if there is a value returned through get request

Before After

Changes proposed in this pull request:

  • Shifted is-input-field helper check at the top of each loop
  • Implemented a check for value is present in get data

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@akshat0047
Copy link
Member Author

Please review the PR, Travis CI is probably failing because of indentation errors, fixing it soon. Is my approach correct? @CosmicCoder96 @mrsaicharan1

Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

  • Semantic Naming for PR.
  • Fix Indents.
  • Check the PR previously made to it and follow the same.
  • Include a GIF showing what changes you have made and details you filled. (Before/After).
  • Show me the rendering of gender field

@akshat0047
Copy link
Member Author

* Semantic Naming for PR.
  Will edit name once I implement changes below
* Fix Indents.
  On it
* Check the PR previously made to it and follow the same.
  Ok sure
* Include a GIF showing what changes you have made and details you filled. (Before/After).
 I have added hyper link with name Before and After, there was some issue with markup showing up my images
* Show me the rendering of `gender field`
    Fixing this

@kushthedude
Copy link
Member

Update?

@akshat0047
Copy link
Member Author

I was away for a while due to festive season, back on it and will give you the updates asap.

@akshat0047 akshat0047 changed the title added check for value present fix : added check for required-fields and rendering gender Oct 30, 2019
@akshat0047
Copy link
Member Author

akshat0047 commented Oct 30, 2019

I have updated the changes suggested

  • semantic naming for PR
  • fixing indents
  • rendering gender (there was a bug in input-type helper)
  • fixing Travis CI failure
  • Adding gif of change(below)

Please review @CosmicCoder96 @kushthedude @mrsaicharan1

  • here City and State are optional fields
    Frontend Fix

@akshat0047
Copy link
Member Author

@iamareebjamal I have fixed the changes required please review, already requested active reviewers on the project

@iamareebjamal
Copy link
Member

Thanks. Can you please confirm that if you remove the required check from frontend, are you able to successfully submit the form or not

@kushthedude kushthedude changed the title fix : added check for required-fields and rendering gender fix: added check for required-fields and rendering gender Nov 3, 2019
@auto-label auto-label bot added the fix label Nov 3, 2019
@fossasia fossasia deleted a comment Nov 3, 2019
@akshat0047
Copy link
Member Author

@iamareebjamal the change is not related to form submission, I have changed the way things are rendering in the attendee section i.e. in the attendee list template

@fossasia fossasia deleted a comment Nov 8, 2019
@akshat0047
Copy link
Member Author

@iamareebjamal Please let me know if I am wrong with my facts, the form was getting submitted before I added the required field, my workflow was:

  • Added an event
  • Created tickets for free
  • Buyed two tickets for two different attendees one with only required fields and another with req. + extra fields
  • After applying the check in attendee-list only fields marked as required while creating the attendee form was rendered
  • Rendered in the terms of Attendee dashboard in my events

Please merge my Pull Request if this is the correct approach.

@akshat0047
Copy link
Member Author

Thanks. Can you please confirm that if you remove the required check from frontend, are you able to successfully submit the form or not

I have checked this and yes we are able to submit the form successfully. Created two tickets with both required and non-required fields, working is as shown in initial screenshots.

@fossasia fossasia deleted a comment Nov 11, 2019
@iamareebjamal
Copy link
Member

Then it needs to be fixed on server. Please open an issue on server

@akshat0047
Copy link
Member Author

ok opening an issue on server for required fields but I think I should modify this for correcting the rendering of gender field?

@iamareebjamal
Copy link
Member

@kushthedude Will be able to tell better

@akshat0047
Copy link
Member Author

@kushthedude Please suggest me if I should modify the PR in the context mentioned above

@kushthedude kushthedude self-requested a review November 13, 2019 03:19
@akshat0047 akshat0047 force-pushed the required-fields branch 2 times, most recently from 7b9aa55 to 5c6d1f1 Compare November 24, 2019 06:05
iamareebjamal
iamareebjamal previously approved these changes Nov 24, 2019
@iamareebjamal
Copy link
Member

@kushthedude Please confirm

@niranjan94
Copy link
Member

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

Complexity increasing per file
==============================
- app/helpers/is-input-field.js  1
         

See the complete overview on Codacy

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.

Attendee List: Only render required fields and some of the fields are not rendering.
4 participants