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

custom fields: Clean custom fields to use existing defined function. #9052

Closed
wants to merge 1 commit into from

Conversation

YJDave
Copy link
Collaborator

@YJDave YJDave commented Apr 10, 2018

Testing Plan:

GIFs or Screenshots:
customprofilefieldedit

@YJDave
Copy link
Collaborator Author

YJDave commented Apr 10, 2018

Some more observations, should be addressed soon:

  • Small fixes required in modal i.e. clear data when add new field,
  • In custom fields, "ab" and "ab " are handled as different fields, not sure if this is an issue. We don't compare names with space trim operation I think.
  • Proper backend error messages. Currently for any backend error, there is only one message "Name already exist" on integrity error, even if there do not exist any similar name field.

@timabbott
Copy link
Sponsor Member

@YJDave can you fix the Casper tests that failed? They should be changed to check for the new alerts; check some of Shubham Dhama's commits if needed.

@timabbott
Copy link
Sponsor Member

(Otherwise this looks like a really nice cleanup; and I agree with the cleanups).

@YJDave
Copy link
Collaborator Author

YJDave commented Apr 12, 2018

Thanks for reviewing! Tests are passing now.

@timabbott
Copy link
Sponsor Member

Merged, thanks @YJDave! I love it when we get to delete code :)

@timabbott timabbott closed this Apr 12, 2018
@YJDave YJDave deleted the cf_settings branch April 12, 2018 16:42
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.

None yet

3 participants