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.

Sign up for GitHub

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

Jump to bottom

fix: Set role and confirm password while adding user mandatory #1758

Merged
merged 5 commits into from
Jan 6, 2022
Merged

fix: Set role and confirm password while adding user mandatory #1758

merged 5 commits into from
Jan 6, 2022

Conversation

mayurnewase
Copy link
Contributor

@mayurnewase mayurnewase commented Dec 13, 2021 •
edited
Loading

Description

Role and Confirm password field in user db model view were not marked as required field, so added those validators.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

@codecov
Copy link

codecov bot commented Dec 13, 2021 •
edited
Loading

Codecov Report

Merging #1758 (f94d7cc) into master (baddd7c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f94d7cc differs from pull request most recent head 4883065. Consider uploading reports for the commit 4883065 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1758   +/-   ##
=======================================
  Coverage   76.88%   76.88%           
=======================================
  Files          56       56           
  Lines        8136     8137    +1     
=======================================
+ Hits         6255     6256    +1     
  Misses       1881     1881
Flag Coverage Δ
python 76.88% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flask_appbuilder/security/views.py 62.26% <100.00%> (+0.10%) ⬆️

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 afc8e2c...4883065. Read the comment docs.

@dpgaspar
Copy link
Owner

Hi, thank you for contributing.

Please install the requirements-dev.txt and run black to pass CI

@dpgaspar dpgaspar self-requested a review December 13, 2021 08:59
dpgaspar
dpgaspar reviewed Dec 13, 2021
View reviewed changes
Copy link
Owner

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

CI is failling

@mayurnewase mayurnewase changed the title Set role and confirm password on db based registration mandatory Fix: Set role and confirm password on db based registration mandatory Dec 13, 2021
@mayurnewase mayurnewase changed the title Fix: Set role and confirm password on db based registration mandatory Fix: Set role and confirm password while adding user mandatory Dec 13, 2021
@mayurnewase mayurnewase changed the title Fix: Set role and confirm password while adding user mandatory fix: Set role and confirm password while adding user mandatory Dec 13, 2021
@mayurnewase mayurnewase requested a review from dpgaspar December 13, 2021 11:28
dpgaspar
dpgaspar reviewed Dec 15, 2021
View reviewed changes
Copy link
Owner

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks good can you add a test to assert user's can't add a user without a role

@mayurnewase mayurnewase requested a review from dpgaspar December 21, 2021 11:47
@dpgaspar dpgaspar merged commit 4489c38 into dpgaspar:master Jan 6, 2022
@dpgaspar dpgaspar mentioned this pull request Jan 6, 2022
Merged
6 tasks
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 9, 2022
@potiuk
Add roles to create_user test
92f8243 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)
@potiuk potiuk mentioned this pull request Jan 9, 2022
Merged
potiuk added a commit to apache/airflow that referenced this pull request Jan 10, 2022
@potiuk
Add roles to create_user test (#20773)
a1a32f7 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)
potiuk added a commit to apache/airflow that referenced this pull request Jan 22, 2022
@potiuk
Add roles to create_user test (#20773)
bcf18e5 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

(cherry picked from commit a1a32f7)
jedcunningham pushed a commit to apache/airflow that referenced this pull request Jan 27, 2022
@potiuk @jedcunningham
Add roles to create_user test (#20773)
6f4d29c 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

(cherry picked from commit a1a32f7)
@mayurnewase mayurnewase mentioned this pull request Feb 23, 2022
Merged
6 tasks
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
@potiuk
Add roles to create_user test (#20773)
8845dfb 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

(cherry picked from commit a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0)

GitOrigin-RevId: 6f4d29cee2fea277eaaa11490cb5871a788dd018
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 10, 2022
@potiuk
Add roles to create_user test (#20773)
fe04dcf 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
@potiuk
Add roles to create_user test (#20773)
7cc7921 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
@potiuk
Add roles to create_user test (#20773)
1922ccc 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
@potiuk
Add roles to create_user test (#20773)
cad6faf 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
@potiuk
Add roles to create_user test (#20773)
fc2b1aa 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
@potiuk
Add roles to create_user test (#20773)
8c1c73d 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
@potiuk
Add roles to create_user test (#20773)
6be6608 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2024
@potiuk
Add roles to create_user test (#20773)
a63d96c 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 7, 2024
@potiuk
Add roles to create_user test (#20773)
7971734 
Flask App Builder 3.4.3 made role and conf_password obligatory
when creating user:
dpgaspar/Flask-AppBuilder#1758

Our test for user creation did not have the role set (though the
UI used it and the cient also allows to set it).

This change adds the `roles` in our tests to enable upgrade
to FAB 3.4.3 for the CI (currently tests in main fail because of
the test failure)

GitOrigin-RevId: a1a32f7c7c2df41e6150f2594a3a41bfecb76fb0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgaspar dpgaspar dpgaspar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mayurnewase @dpgaspar