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 position form organization being required #3366

Merged
merged 4 commits into from
Dec 21, 2020

Conversation

cemalettin-work
Copy link
Contributor

@cemalettin-work cemalettin-work commented Dec 18, 2020

Currently, position form does not show to the users (admins/ super-users) that organization field is required.

So when an admin or a super-user tries to submit a position with only position name ( as it is the only one shown as required), server returns a response: "You do not have permissions to do this". In reality, the user has permission but organization being missing would result into a fail in isSuperUserForOrg. We should return something along the lines of "organization is required" response.

Also, editPosition.specs.js file and tests are not editing any position but creating, which is confusing. It goes to the create position page, so i named the tests/pages accordingly. Also added some test cases for required fields.

Also fixes:

  • Changing organization type does not immediately change Organization field search query, so the list shows Principal orgs when on advisor etc.

User changes

Super User changes

  • Super users will see that organization is a required field warning.

Admin changes

  • Admins will see that organization is a required field warning.

System admin changes

  • anet.yml or anet-dictionary.yml needs change
  • db needs migration
  • documentation has changed
  • graphql schema has changed

Checklist

  • Described the user behavior in PR body
  • Referenced/updated all related issues
  • commits follow a repo#issue: Title title format and these 7 rules
  • commits have a clean history, otherwise PR may be squash-merged
  • Added and/or updated unit tests
  • Added and/or updated e2e tests
  • Added and/or updated data migrations
  • Updated documentation
  • Resolved all build errors and warnings
  • Opened debt issues for anything not resolved here

@cemalettin-work cemalettin-work force-pushed the fix-position-form-org-required branch 4 times, most recently from ab38dd7 to 9e1e0f3 Compare December 18, 2020 09:37
@cemalettin-work cemalettin-work force-pushed the fix-position-form-org-required branch 5 times, most recently from c5c843b to 309d78e Compare December 21, 2020 05:29
…e request

As otherwise we would never get to the required organization exception
It is probably a Yup or Formik bug, this is a workaround
Add create position test cases for required fields
@cemalettin-work cemalettin-work force-pushed the fix-position-form-org-required branch from 309d78e to 8a813d8 Compare December 21, 2020 08:26
client/src/models/Position.js Outdated Show resolved Hide resolved
…e changes

Also make required fields' message consistent and concise

State update should be done with setFieldValue not by mutating 'values'
@gjvoosten gjvoosten merged commit f28f7b8 into candidate Dec 21, 2020
@gjvoosten gjvoosten deleted the fix-position-form-org-required branch December 21, 2020 14:11
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.

3 participants