[admin] Removed logic duplicates #40 #41
Conversation
NoumbissiValere
force-pushed
the
issue/40
branch
3 times, most recently
from
7e4e409
to
bb8dd77
Compare
|
@nemesisdesign Please can you can review this when you have time so that i make corrections if needed. I have equally done the modifications in the other |
| class Media: | ||
| js = ('openwisp-utils/js/uuid.js',) | ||
|
|
||
| def _get_fields(self, fields, request, obj=None): |
There was a problem hiding this comment.
why are you adding this code and the lines below?
There was a problem hiding this comment.
I adding those lines so that the readonly fields (uuid in this case) should be should come up only in the change_view. without this, the uuid field will appear in the add_view with a None value. Equally this was the same approach used in OrganizationAdmin found in the openwisp_users repo.
| fields = ['name', 'key', 'uuid'] | ||
|
|
||
| class Media: | ||
| js = ('openwisp-utils/js/uuid.js',) |
There was a problem hiding this comment.
does it work also without adding openwisp_utils to INSTALLED_APPS? Can it actually be loaded in the browser correctly? Did you try running the test project in the browser and check this?
NoumbissiValere
force-pushed
the
issue/40
branch
from
bb8dd77
to
1d51200
Compare
|
@nemesisdesign Please can you look at the receive_url i added. i wish to know if it's ok before i add the tests. Even though i feel that |
NoumbissiValere
force-pushed
the
issue/40
branch
from
1d51200
to
1a5ac0a
Compare
Removed the logics which were duplicated in the others modules and redefined it here to enable reusability and ease debugging. Fixes openwisp#40
NoumbissiValere
force-pushed
the
issue/40
branch
from
1a5ac0a
to
6506253
Compare
There was a problem hiding this comment.
This looks good but has to be tested and I haven't had time, it's been sitting here for a while.
@atb00ker could you help me out to test this? If we merge this, we can then create issues (and GCI tasks) in other modules to remove the duplication and use the new code here.
| field.click(function () { | ||
| $(this).select(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This needs to be }); uneven brackets. 😄
| class Media: | ||
| js = ('openwisp-utils/js/uuid.js',) | ||
|
|
||
| def _get_fields(self, fields, request, obj=None): | ||
| """ | ||
| removes readonly_fields in add view | ||
| """ | ||
| if obj: | ||
| return fields | ||
| new_fields = fields[:] | ||
| for field in self.readonly_fields: | ||
| if field in new_fields: | ||
| new_fields.remove(field) | ||
| return new_fields |
There was a problem hiding this comment.
Can't this be generalized and added to the UUIDAdmin class?
There was a problem hiding this comment.
Correct, I think this should be generalized in the UUIDAdmin class.
There was a problem hiding this comment.
I'm leaving this review in case somebody will pick this up.
Another improvement we can do to avoid having to add openwisp_utils to INSTALLED_APPS:
we can patch DependencyFinder to automatically add openwisp_utils in the dependencies, that way the static files of openwisp-utils will be available without having to add it to INSTALLED_APPS, which would require changing other projects like ansible-openwisp2 and docker-openwisp.
I also think this improvement makes sense since adding openwisp_utils to INSTALLED_APPS to load is static files even though we are explicitly using its own staticfile finder class is redundant.
| class Media: | ||
| js = ('openwisp-utils/js/uuid.js',) | ||
|
|
||
| def _get_fields(self, fields, request, obj=None): | ||
| """ | ||
| removes readonly_fields in add view | ||
| """ | ||
| if obj: | ||
| return fields | ||
| new_fields = fields[:] | ||
| for field in self.readonly_fields: | ||
| if field in new_fields: | ||
| new_fields.remove(field) | ||
| return new_fields |
There was a problem hiding this comment.
Correct, I think this should be generalized in the UUIDAdmin class.
|
Thanks a lot @NoumbissiValere for your work, we merged your initial work after some improvements in PR #71. 👍 |
Removed the logics which were duplicated
in the others modules and redefined it here to enable reusability and ease debugging.
Fixes #40