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

[change] Changed the command to delete_old_radiusbatch_users #496 #502

Merged
merged 1 commit into from
Feb 22, 2024
Merged

[change] Changed the command to delete_old_radiusbatch_users #496 #502

merged 1 commit into from
Feb 22, 2024

Conversation

praptisharma28
Copy link
Member

@praptisharma28 praptisharma28 commented Dec 26, 2023 •
edited
Loading

Changed delete_old_users command to delete_old_radiusbatch_users to help users understand the purpose of the command more clearly.
Closes #496

@praptisharma28
Copy link
Member Author

Hey @pandafy , please review the PR.

pandafy
pandafy requested changes Dec 27, 2023
View reviewed changes
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@praptisharma28 for future work, don't create new PRs for the same issue. You can update the PR by pushing (or force pushing) to the same branch. Creating multiple branches causes problems in reviews. If you are new to git, I highly recommend checking https://learngitbranching.js.org/.

I still found occurrences of delete_old_users in the code

$ git grep 'delete_old_users' praptisharma28-commandchange 
praptisharma28-commandchange:docs/source/user/management_commands.rst:    ./manage.py delete_old_users --older-than-months <duration_in_months>
praptisharma28-commandchange:tests/openwisp2/sample_radius/management/commands/delete_old_radiusbatch_users.py:from openwisp_radius.management.commands.base.delete_old_users import (

Please pay attention to the requirements of the issue. We need to change all occurrences. Do a project wide search and change all occurrences.

This hints me that you didn't run the tests locally. Also, the QA checks are failing. Please run QA checks and tests locally every time before opening/updating a PR.

@praptisharma28
Copy link
Member Author

@pandafy , This qa checks didnt fail when I checked, may be something happened in the process, I am looking into it again, also I made an another branch and closed the previous PR because previously force pushing lead to PR check failure + I tried to now install linux and contribute, hence I created a new PR. Otherwise I would have forced push.

@praptisharma28
Copy link
Member Author

praptisharma28 commented Dec 27, 2023 •
edited
Loading

Done @pandafy , commited the change, have a look again. Also why do the coveralls fail, the commit is written correctly right? I have done the same way earlier, they didnt fail at that time.

pandafy
pandafy requested changes Dec 27, 2023
View reviewed changes
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@praptisharma28 the QA checks are still failing, see here https://github.com/openwisp/openwisp-radius/actions/runs/7337741815/job/19985516754?pr=502#step:8:139

Run openwisp-qa-format command to format the code properly.

@praptisharma28
Copy link
Member Author

@pandafy , please check the latest commits.

@pandafy
Copy link
Member

pandafy commented Jan 15, 2024

@praptisharma28 the build is till failing.

@praptisharma28 @pandafy
[change!] Changed "delete_old_users" to "delete_old_radiusbatch_users" …
950b9bc 
…openwisp#496

Changed "delete_old_users command" to "delete_old_radiusbatch_users" to help
users understand the purpose of the command more clearly.

Closes openwisp#496
@coveralls
Copy link

Coverage Status

coverage: 98.723%. remained the same
when pulling 950b9bc on praptisharma28:commandchange
into 67fb112 on openwisp:master.

@pandafy pandafy requested a review from nemesifier February 19, 2024 12:39
@pandafy pandafy self-requested a review February 19, 2024 12:39
nemesifier
nemesifier reviewed Feb 20, 2024
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, @pandafy / @praptisharma28 we probably need to update ansible-openwisp2 and docker-openwisp right? If I am not mistaken this command is used there.

Can you please double check and create the issues?

@nemesifier
Copy link
Member

@pandafy @praptisharma28 can you post the links to the opened issues here please?

nemesifier
nemesifier approved these changes Feb 22, 2024
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.

I see the issue has been linked to the issue linked to this PR, here's the issues for reference:

@nemesifier nemesifier merged commit eeed16b into openwisp:master Feb 22, 2024
20 checks passed
nemesifier added a commit to openwisp/ansible-openwisp2 that referenced this pull request Mar 5, 2024
@nemesifier
[change!] Renamed delete_old_users to delete_old_radiusbatch_users
6013477 
Related to openwisp/openwisp-radius#502
nemesifier added a commit to openwisp/ansible-openwisp2 that referenced this pull request Mar 5, 2024
@nemesifier
[change!] Renamed delete_old_users to delete_old_radiusbatch_users #463
29861b5 
Related to openwisp/openwisp-radius#502
Closes #463
@nemesifier nemesifier mentioned this pull request Mar 5, 2024
Merged
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 9, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
e27fcf9 
…p#294

Updated the command. Related to openwisp/openwisp-radius#502 .This change ensures proper functionality and maintains consistency.

Closes openwisp#294
@praptisharma28 praptisharma28 mentioned this pull request Mar 9, 2024
Merged
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
e43d2fe 
…p#294

Updated the command. Related to openwisp/openwisp-radius#502

Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
Changed the command to delete_old_radiusbatch_users openwisp#294
1b89a79 
Updated the command. Related to openwisp/openwisp-radius#502

Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
a606d2d 
…p#294

Updated the command. Related to openwisp/openwisp-radius#502

Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
7cbbc81 
…p#294\n\nUpdated the command. Related to openwisp/openwisp-radius#502\n\nCloses openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
0438c73 
…p#294                                                                                                                                    Updated the command. Related to openwisp/openwisp-radius#502                                                                                                                                                                                                  Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
827a95d 
…p#294                                                                                                                                      Updated the command. Related to openwisp/openwisp-radius#502                                                                                                                                                                                                Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
82b87f1 
…p#294

                                                                                                                                     Updated the command. Related to openwisp/openwisp-radius#502
                                                                                                                                                                                              Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
7ba20aa 
…p#294

Updated the command. Related to openwisp/openwisp-radius#502

Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change!] Changed the command to delete_old_radiusbatch_users openwis…
d64cf38 
…p#294

Updated the command. Related to openwisp/openwisp-radius#502
Closes openwisp#294
praptisharma28 added a commit to praptisharma28/docker-openwisp that referenced this pull request Mar 12, 2024
@praptisharma28
[change] Changed the command openwisp#294
e52b3c1 
Updated the command. Related to openwisp/openwisp-radius#502
Closes openwisp#294
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

@pandafy pandafy Awaiting requested review from pandafy

Assignees
No one assigned
Labels
None yet
Projects
OpenWISP Contributor's Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[change] Change name of delete_old_users command to reflect its functionality accurately
4 participants
@praptisharma28 @pandafy @coveralls @nemesifier