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] Add user defined properties field in Node and Link #85 #91

Merged
merged 1 commit into from
Sep 18, 2020

Conversation

NoumbissiValere
Copy link
Contributor

@NoumbissiValere NoumbissiValere commented Sep 1, 2020

Closed issue by adding a user defined properties field to Node and Link model
which users could use to defined their own properties as imported properties
are now readonly.

Closes #85

@NoumbissiValere NoumbissiValere self-assigned this Sep 1, 2020
@NoumbissiValere NoumbissiValere changed the title [change] Add set of properties meant to be manipulated by the user in… [change] Add user defined properties field in Node and Link #85 Sep 1, 2020
@NoumbissiValere NoumbissiValere marked this pull request as draft September 2, 2020 01:02
@NoumbissiValere NoumbissiValere force-pushed the issue-85-add-properties-field branch 2 times, most recently from 2290eba to 9cdbc70 Compare September 3, 2020 20:10
@TravisBuddy

This comment has been minimized.

@coveralls
Copy link

coveralls commented Sep 3, 2020

Coverage Status

Coverage increased (+0.02%) to 99.057% when pulling e418f4d on issue-85-add-properties-field into 7e8b97a on master.

@NoumbissiValere NoumbissiValere marked this pull request as ready for review September 3, 2020 20:18
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.

Great progress!

Can you please do something to display the properties (on node and link admin change page) in a nicer way?

Screenshot from 2020-09-06 16-41-48

An admin method that simply adds <br/> elements would do or anything that makes it look better.

See my other comments below.

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.

Great progress!

Can you please do something to display the properties (on node and link admin change page) in a nicer way?

An admin method that simply adds <br/> elements would do or anything that makes it look better.

Thanks for trying something with the properties but that's not what I meant.

Please try something like the following:

from django.utils.safestring import mark_safe

@admin.register(Node)
class NodeAdmin(NodeLinkMixin, BaseAdmin):
    # ... 
    
    fields = [
        # ... omitted other fields
        'readonly_properties',
    ]
    readonly_fields = ['readonly_properties']

    def readonly_properties(self, obj):
        output = ''
        for key, value in obj.properties.items():
            key = key.replace('_', ' ').capitalize()
            output += f'<p><strong>{key}</strong>: {value}</p>'
        return mark_safe(output)

    readonly_properties.short_description = _('properties')

This is working code but please ensure it's shared by NodeAdmin and LinkAdmin (do not duplicate it in both classes).

It looks like this:

Screenshot from 2020-09-15 19-22-56

Please also update the tests accordingly.

@NoumbissiValere NoumbissiValere force-pushed the issue-85-add-properties-field branch 2 times, most recently from 6552d5a to 9daec18 Compare September 16, 2020 20:35
from django.utils.translation import ugettext_lazy as _
from flat_json_widget.widgets import FlatJsonWidget
Copy link
Member

Choose a reason for hiding this comment

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

this module needs to be added here: https://github.com/openwisp/openwisp-network-topology/blob/master/requirements.txt

openwisp-controller is not a direct dependency. The build is not failing because openwisp-controller is being installed with an additional command to test the integration module.

Copy link
Contributor Author

@NoumbissiValere NoumbissiValere Sep 17, 2020

Choose a reason for hiding this comment

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

@nemesisdesign Thank you for this catch. I forgot to add it 😄 . But the build isn't failing because of this. the Build is failing because modify_settings is called. I tried some many fix but they didn't work. I don't think the fix should be done in this module. if we fix it here, we might be hiding a potential bug which will certainly surface again one day. My master branch is equally failing with the same error.

@openwisp openwisp deleted a comment from TravisBuddy Sep 17, 2020
@openwisp openwisp deleted a comment from TravisBuddy Sep 17, 2020
@openwisp openwisp deleted a comment from TravisBuddy Sep 17, 2020
@openwisp openwisp deleted a comment from TravisBuddy Sep 17, 2020
@NoumbissiValere
Copy link
Contributor Author

@nemesisdesign I will rebase this once #93 is merged

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! 👍

@nemesifier nemesifier merged commit 83388e2 into master Sep 18, 2020
@nemesifier nemesifier deleted the issue-85-add-properties-field branch September 18, 2020 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[change] Add set of properties meant to be manipulated by the user in Node and Link
4 participants