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

[extending] Extend openwisp-users #82 #86

Merged
merged 6 commits into from
Apr 3, 2021
Merged

[extending] Extend openwisp-users #82 #86

merged 6 commits into from
Apr 3, 2021

Conversation

devkapilbansal
Copy link
Member

Closes #82

@coveralls
Copy link

coveralls commented Mar 11, 2021 •
edited
Loading

Coverage Status

Coverage remained the same at 99.377% when pulling ad3f1f0 on issues/82-extend-openwisp-users into 687d9a7 on master.

nemesifier
nemesifier reviewed Mar 11, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier 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, is anything missing?

@nemesifier
Copy link
Member

Oh yes, the we discussed we should be adding a sample_user app when SAMPLE_APP=1 to ensure the extension is really working.

@devkapilbansal devkapilbansal force-pushed the issues/82-extend-openwisp-users branch 2 times, most recently from 8047e7a to da9ccc9 Compare March 12, 2021 08:58
@devkapilbansal devkapilbansal force-pushed the issues/82-extend-openwisp-users branch 7 times, most recently from ecb703d to ca30016 Compare March 20, 2021 22:19
@devkapilbansal devkapilbansal marked this pull request as ready for review March 20, 2021 22:20
@devkapilbansal devkapilbansal added this to In progress in OpenWISP Priorities for next releases via automation Mar 21, 2021
@devkapilbansal devkapilbansal moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Mar 21, 2021
@devkapilbansal devkapilbansal force-pushed the issues/82-extend-openwisp-users branch from 49eb131 to 66bb591 Compare March 22, 2021 07:18
@nemesifier nemesifier added the enhancement New feature or request label Mar 24, 2021
nemesifier
nemesifier reviewed Mar 24, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier 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! Is there any integration we can do to the README? I see no line of docs is updated, may be good to at least mention something about this? Eg: we do this to test the integration and extension of openwisp-users but it is not required. Something like that or if you have any other idea about what may be useful for users of this module, please add it.

@atb00ker can you take a look at this please?

atb00ker
atb00ker reviewed Mar 25, 2021
View reviewed changes
Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

Very clean work, I like it. 😊
Some questions:

openwisp_ipam/migrations/__init__.py Show resolved Hide resolved
tests/openwisp2/sample_ipam/tests.py Show resolved Hide resolved
tests/openwisp2/settings.py Show resolved Hide resolved
devkapilbansal
devkapilbansal commented Mar 25, 2021
View reviewed changes
openwisp_ipam/migrations/__init__.py Show resolved Hide resolved
tests/openwisp2/sample_ipam/tests.py Show resolved Hide resolved
tests/openwisp2/sample_users/tests.py Show resolved Hide resolved
tests/openwisp2/settings.py Show resolved Hide resolved
@devkapilbansal devkapilbansal changed the title [extending] WIP Extend openwisp-users #82 [extending] Extend openwisp-users #82 Mar 25, 2021
@atb00ker atb00ker mentioned this pull request Mar 27, 2021
Merged
nemesifier
nemesifier reviewed Mar 31, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks @atb00ker for the review, do you think we can merge?

OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress Mar 31, 2021
@atb00ker
Copy link
Member

Thanks @atb00ker for the review, do you think we can merge?

I think we should take care of Integration tests first! 😄

@devkapilbansal , as mentioned by Fed, integration tests are important.
Can you please add them back?
I am expecting some problems to appear, let me know if you need help with that! 😄

nemesifier
nemesifier requested changes Apr 1, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Please rebase on the current master

@devkapilbansal devkapilbansal force-pushed the issues/82-extend-openwisp-users branch 2 times, most recently from 6a0442a to 1407d3c Compare April 3, 2021 00:13
atb00ker
atb00ker previously approved these changes Apr 3, 2021
View reviewed changes
Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

I think the changes look good if we don't need the integration tests! 😄

@devkapilbansal
Copy link
Member Author

Looks good! Is there any integration we can do to the README? I see no line of docs is updated, may be good to at least mention something about this? Eg: we do this to test the integration and extension of openwisp-users but it is not required. Something like that or if you have any other idea about what may be useful for users of this module, please add it.

@atb00ker can you take a look at this please?

@atb00ker do I have to make some changes to README too as mentioned here

atb00ker
atb00ker reviewed Apr 3, 2021
View reviewed changes
Copy link
Member

@atb00ker atb00ker left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@atb00ker do I have to make some changes to README too as mentioned here

Oops, I thought, I already commented about that.

Can you look at openwisp-radius here: https://openwisp-radius.readthedocs.io/en/latest/developer/how_to_extend.html?highlight=sample_users#extending-openwisp-radius

If you want to add new users fields, please follow the tutorial to extend the openwisp-users. As an example, we have extended openwisp-users to sample_users app and added a field social_security_number in the sample_users/models.py.

This should enable user to get started with extending user! 😄

atb00ker
atb00ker reviewed Apr 3, 2021
View reviewed changes
Copy link
Member

@atb00ker atb00ker left a comment •
edited
Loading

Choose a reason for hiding this comment

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

** Change in README required as suggested: #86 (review)

@atb00ker atb00ker self-requested a review April 3, 2021 12:05
@devkapilbansal devkapilbansal moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Apr 3, 2021
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to Reviewer approved Apr 3, 2021
nemesifier
nemesifier approved these changes Apr 3, 2021
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks @devkapilbansal

@nemesifier nemesifier merged commit 8cf050d into master Apr 3, 2021
OpenWISP Priorities for next releases automation moved this from Reviewer approved to Done Apr 3, 2021
@devkapilbansal devkapilbansal deleted the issues/82-extend-openwisp-users branch April 4, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemesifier nemesifier nemesifier approved these changes

@atb00ker atb00ker Awaiting requested review from atb00ker

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
OpenWISP Priorities for next releases
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] Load openwisp-users models using swappable
4 participants
@devkapilbansal @coveralls @nemesifier @atb00ker