[feature] Topology history #49
Conversation
rohithasrk
force-pushed
the
topology-history
branch
from
0718145 to
74c94c5
Compare
| """ | ||
| Saves the snapshot of topology | ||
| """ | ||
| Snapshot = self.snapshot_model |
There was a problem hiding this comment.
Snapshot = will make flake8 complain
There was a problem hiding this comment.
Didn't get you @nemesisdesign. There are no errors in the travis as of now
There was a problem hiding this comment.
you are right, I don't know why I was tricked by my memory. Some code analysis tool complain if you declare a CamelCase variables, but not the default flake8 configuration, ignore this comment
| s.data = self.json() | ||
| else: | ||
| s = Snapshot(topology=self, data=self.json()) | ||
| s.save() |
There was a problem hiding this comment.
Why not:
date = datetime.now().date()
options = dict(topology=self, date=date)
try:
s = Snapshot.objects.get(**options)
else:
s = Snapshot(**options)
s.data = self.json()
s.save()There was a problem hiding this comment.
This looks much better.
django_netjsongraph/base/topology.py
Outdated
| - save snapshots of topoogies | ||
| - logs failures | ||
| """ | ||
| queryset = cls.objects.all() |
There was a problem hiding this comment.
change to:
queryset = cls.objects.filter(published=True)
rohithasrk
force-pushed
the
topology-history
branch
from
74c94c5 to
07b1e73
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@rohithasrk you have improved a lot but you need to start writing tests sooner in the process to become a great developer. See my inline comments and then please start adding tests
|
|
||
| def get(self, request, pk, format=None): | ||
| topology = get_object_or_404(self.topology_model, pk) | ||
| date = request.query_params.get('date') |
There was a problem hiding this comment.
write a test in which you supply a rubbish date here, make it fail and then find a fix (great occasion to learn TDD)
There was a problem hiding this comment.
Yeah. I get it 😄
django_netjsongraph/api/generics.py
Outdated
|
|
||
| def post(self, request, pk, format=None): | ||
| topology = get_object_or_404(self.model, pk, strategy='receive') | ||
| topology = get_object_or_404(self.topology_model, pk, strategy='receive') |
There was a problem hiding this comment.
I don't understand: why do we need to change this line? I think we can keep self.model because we don't need to handle different models in this view
There was a problem hiding this comment.
True that. I thought it would just make it a little more uniform.
There was a problem hiding this comment.
as a general rule, never change lines which are working fine and do not need to be changed, unless you have a very valid reason to do so
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@rohithasrk great progress 👍 see my inline comments, then proceed with the remaining tasks
| return '/api/receive/{0}/?key=test'.format(t.pk) | ||
|
|
||
| def _set_receive(self, topology_model): | ||
| t = topology_model.objects.first() |
There was a problem hiding this comment.
you could make this a property by prepending @property to it, then you can call self.snapshot_url
There was a problem hiding this comment.
Oh yes. Will do it
| t.expiration_time = 0 | ||
| t.save() | ||
|
|
||
| def snapshot_date(self): |
There was a problem hiding this comment.
same here, it makes sense to use the @property decorator here too
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@rohithasrk good progress 👍
what's next?
There was a problem hiding this comment.
👍 great progress @rohithasrk, see my inline comments.
PS: here's more feedback after having tested the changes.
- is the CSS of the datepicker widget missing or what? I get an unstyled widget in my test env
- put the date field in an overlay using the same CSS classes used for the upper right and left overlay and adapt the style so
- try to set the default date to the current date and edit the javascript so that if the date set is equal to the current date the date parameter sent to the API is empty, that way we get the latest data in the default case but for users it will be easier to understand they can change the date; any other small usability improvement is appreciated!
|
|
||
| def visualize_view(self, request, pk): | ||
| topology = get_object_or_404(self.model, pk) | ||
| api_url = reverse('network_graph', args=[pk]) |
| } | ||
| date = date.join('-'); | ||
| graph_url = $('.switcher').attr('graph-url')+'?date='+date; | ||
| console.log(graph_url); |
1 similar comment
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Have you added the missing CSS yet?
| <p><span class="link down"> </span>link down</p> | ||
| </div> | ||
| <div class="switcher" graph-url='{{graph_url}}'> | ||
| <label for="datepicker"> Date</label><input type="text" id="datepicker"> |
There was a problem hiding this comment.
is the space before the word Date necessary?
rohithasrk
force-pushed
the
topology-history
branch
from
d931bc5 to
a69da2d
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
great! You are almost there, see my inline comments
| visualize_url = $('.visualizelink').attr('data-url'); | ||
|
|
||
| var d = new Date(), | ||
| month = d.getMonth()+1, |
| var d = new Date(), | ||
| month = d.getMonth()+1, | ||
| day = d.getDate(), | ||
| default_date = (month<10? '0':'') + month + '/' + |
There was a problem hiding this comment.
change to month < 10 ? '0' : ''
| month = d.getMonth()+1, | ||
| day = d.getDate(), | ||
| default_date = (month<10? '0':'') + month + '/' + | ||
| (day<10? '0':'') + day + '/' + |
There was a problem hiding this comment.
add spaces between operands, eg: day < 10 ? '0' : ''
| date[2] = x; | ||
| } | ||
| date = date.join('-'); | ||
| graph_url = $('.switcher').attr('graph-url')+'?date='+date; |
There was a problem hiding this comment.
same, here separate string operands and plus signs with spaces
| error(function(xhr) { | ||
| if (xhr.status == 400) { | ||
| } | ||
| }); |
There was a problem hiding this comment.
what about this section? can this situation be triggered by the user? If yes, add an alert with some text message, otherwise remove it
There was a problem hiding this comment.
Yes. This will be triggered when a date ahead is chosen. So yes. Will add an alert
| current_date = d.getFullYear() + '-' + | ||
| (month<10? '0':'') + month + '-' + | ||
| (day<10? '0':'') + day; | ||
| $('#submit').click(function() { |
There was a problem hiding this comment.
instead of the submit event, can you use the change event of the date input? It would be more usable
|
PS: to completely avoid the risk of breaking existing instances, this feature should be released in the 0.4.0 version |
rohithasrk
force-pushed
the
topology-history
branch
from
199669d to
649e67b
Compare
|
@rohithasrk I think one point is still missing:
|
rohithasrk
force-pushed
the
topology-history
branch
from
c16daef to
cd4cc17
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@rohithasrk 👍 great work. I will rebase&merge this and add a small improvement to the UI
|
Greetings from AWMN Silly question: How can I use the snapshot feature? It doesn't appear in the admin panel as an option, while issuing a manage.py save_snapshot on a daily cron job doesn't auto-magically populate the history calendar. |
|
@acoul are you using the latest version? A calendar show automatically show up, eg: https://openwisp.nnxx.ninux.org/topology/topology/36d5ba3e-1364-49d2-9e29-1f6dceb7cbbb/ |
|
yes using latest git version. every night @ 11 a cron executes a "manage.py save_snapshot " when clicking on a previous date at the calendar I get a "no snapshot found for this date" message Edit: it's working just fine. sorry for the noise |
4 participants
References #32. As of now, I've just added a Snapshot model and starting working on
save_snapshotfunction.