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

[change] Removed support for restoring FirmwareBuild objects #173

Merged
merged 8 commits into from
Feb 23, 2022

Conversation

AbhigyaShridhar
Copy link
Contributor

[firmware-upgrader/admin.py/Firmware Build Recovery] Removed reversion as a dependency to remove recovery of deleted builds #170

I removed reversion from requirements.txt and from admin.py to remove the Recovery of deleted firmware builds option.
Fixes #170'

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.

It was not asked in #170 to remove reversion support for everything in firmware upgrader.

It was asked to remove support for FirmwareBuild objects, i.e Build objects.

Start from updating BuildAdmin

class BuildAdmin(BaseVersionAdmin):

Reference django-reversion documentation to learn about VersionAdmin: https://django-reversion.readthedocs.io/en/stable/admin.html

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.

It was not asked in #170 to remove reversion support for everything in firmware upgrader.

It was asked to remove support for FirmwareBuild objects, i.e Build objects.

Start from updating BuildAdmin

class BuildAdmin(BaseVersionAdmin):

Reference django-reversion documentation to learn about VersionAdmin: https://django-reversion.readthedocs.io/en/stable/admin.html

Gagan is correct. There's also a conflict with the latest master, please rebase on the latest master.

AbhigyaShridhar and others added 3 commits January 25, 2022 23:52
Replaced Version Admin with ModelAdmin as a superclass in admin.py/BuildAdmin,
Also removed the reversion_register function which was meant to register the model
associated with BuildAdmin (Firmware Build), for recovery option.

Fixes openwisp#170
In admin.py, replaced BaseVersionAdmin from BaseAdmin,
as a superclass for 'BuildAdmin', as it is no longer
desired to add a recovery option for firmware builds and images

Fixes openwisp#170
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.

The tests are failing due to following code

reversion.register(model=DeviceFirmware, follow=['device', 'image'])

It is required to remove image from the list in above code.

'build',
)
return super().reversion_register(model, **kwargs)
# removed overriding reversion_register as recovery operation is no longer desired
Copy link
Member

Choose a reason for hiding this comment

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

You can also remove this comment

Copy link
Member

Choose a reason for hiding this comment

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

@AbhigyaShridhar can you proceed with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pandafy , I made the requested changes. The branch was already up to date with master though. Please let me know if something else needs to be done.

Apologies for the late reply, was facing some health issues in the past week.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @pandafy , I made the requested changes. The branch was already up to date with master though. Please let me know if something else needs to be done.

Thank you for the update!

Apologies for the late reply, was facing some health issues in the past week.

No worries, get well soon! 😊

@@ -90,7 +90,7 @@ class CategoryFilter(MultitenantOrgFilter):


@admin.register(load_model('Build'))
class BuildAdmin(BaseVersionAdmin):
class BuildAdmin(BaseAdmin):
Copy link
Member

@pandafy pandafy Feb 15, 2022

Choose a reason for hiding this comment

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

We don't want to remove reversion for Build model, but for FirmwareImage model.

Copy link
Member

Choose a reason for hiding this comment

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

Correction: After discussion with @nemesisdesign, we came to the conclusion that we want to remove Build model from reversion. Hence, the above change is correct.

To fix tests, it is required to remove following snippet of code as well

def reversion_register(self, model, **kwargs):
if model == Category:
kwargs['follow'] = (*kwargs['follow'], 'build_set')
return super().reversion_register(model, **kwargs)

@pandafy pandafy added this to In progress in OpenWISP Priorities for next releases via automation Feb 15, 2022
@pandafy pandafy changed the title removed reversion dependency to remove build recovery [change] Removed support for restoring FirmwareBuild objects Feb 15, 2022
AbhigyaShridhar and others added 2 commits February 16, 2022 00:50
Replaced BaseVersionAdmin from BaseAdmin as a super class
in BuildAdmin and removed reversion support from BuildAdmin,
thus no longer allowing recovery of deleted Firmware Builds.

Also removed the overriding of the 'reversion_register' method
in CategoryAdmin to avoid failure of tests.

Fixes openwisp#170
@@ -109,7 +109,7 @@ def test_upgrade_selected_error(self):
def test_upgrade_intermediate_page_related(self):
self._login()
env = self._create_upgrade_env()
with self.assertNumQueries(17):
with self.assertNumQueries(15):
Copy link
Member

Choose a reason for hiding this comment

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

The number of queries changes because we removed VersionAdmin from BuildAdmin class

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.

@AbhigyaShridhar can you rebase this PR on the latest master branch?

'build',
)
return super().reversion_register(model, **kwargs)
# removed overriding reversion_register as recovery operation is no longer desired
Copy link
Member

Choose a reason for hiding this comment

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

@AbhigyaShridhar can you proceed with this?

…#173

Removed an extra comment, also did some minor formating
to make sure all checks in ./run-qa-checks work

fixes openwisp#173
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.

LGTM 👍🏼 ! Thank you for contributing @AbhigyaShridhar

Deferring merge to @nemesisdesign 😄

@pandafy pandafy moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Feb 23, 2022
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to Reviewer approved Feb 23, 2022
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 @AbhigyaShridhar @pandafy! 👍

@nemesifier nemesifier merged commit 999b17e into openwisp:master Feb 23, 2022
OpenWISP Priorities for next releases automation moved this from Reviewer approved to Done Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[change] Remove support for restoring FirmwareBuild objects
3 participants