-
Notifications
You must be signed in to change notification settings - Fork 165
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
Incidents API update #95
Conversation
db66899
to
e021843
Compare
class Meta: | ||
model = ExternalUser | ||
fields = ('app_id', 'external_id', 'owner', 'display_name') | ||
fields = ("app_id", "external_id", "display_name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we were returning the owner user primary key. I took it out, as it's basically meaningless, and we can more easily identify an External User by the other three (we have a UNIQUE constraint on them)
class IncidentSerializer(serializers.HyperlinkedModelSerializer): | ||
reporter = serializers.PrimaryKeyRelatedField(queryset=ExternalUser.objects.all(), required=False) | ||
lead = serializers.PrimaryKeyRelatedField(queryset=ExternalUser.objects.all(), required=False) | ||
class IncidentSerializer(serializers.ModelSerializer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have context on why we were using a HyperlinkedModelSerializer
, but this means we return hyperlinks to other API endpoints instead of primary keys. We don't need either in this serializer, so might as well use the simpler ModelSerializer
"severity", | ||
"start_time", | ||
"summary", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatted, sorted and added a few more fields to the request/response
if 'actions' in expand: | ||
self.fields['action_set'] = ActionSerializer(many=True, read_only=True) | ||
instance.save() | ||
return instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new update method here to control exactly how we're deserializing a PUT request and updating the incident instance.
I removed the previous __init__
function as I don't think we need this functionality, though might well put it back in when I get to the Actions API
@@ -5,7 +5,7 @@ | |||
|
|||
# Routers provide an easy way of automatically determining the URL conf. | |||
router = routers.DefaultRouter() | |||
router.register(r'incidents', IncidentViewSet, base_name='Incidents') | |||
router.register(r'incidents', IncidentViewSet, basename='incident') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed for consistency with the auto-generated base names for the other routes. base_name
is also deprecated in favour of basename
|
||
return Incident.objects.filter(start_time__gte=start, start_time__lte=end) | ||
serializer_class = serializers.IncidentSerializer | ||
pagination_class = pagination.LimitOffsetPagination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIs should be controlling pagination, so remove logic around getting the previous months worth of incidents, and simplify the queryset. We'll sort by newest reported incidents to oldest.
@pytest.fixture() | ||
def api_user(transactional_db): | ||
e = ExternalUserFactory() | ||
return e.owner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used for auth. Any authenticated user will do (though may have to update this in future if we start doing more fine-grained permissions)
|
||
|
||
@factory.django.mute_signals(post_save) | ||
class IncidentFactory(factory.DjangoModelFactory): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use factory_boy to easily generate random test data 🙌
from tests.factories import UserFactory, ExternalUserFactory | ||
|
||
@pytest.fixture | ||
def arf(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐶
assert incident["report_time"] | ||
|
||
# incidents should be in order of newest to oldest | ||
if idx != len(incidents) - 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could store report_time
outside the loop and update/compare it on each iteration to simplify here, not a blocker though.
* Update incidents-list (/api/incidents) * Update incidents with PUT /incidents/<pk> * Tidy up tests * Update incident lead via API * /incidents list should be sorted by report date * Return primary key in incident response * Change deprecated parameter * Add docs for incidents endpoint
* Update incidents-list (/api/incidents) * Update incidents with PUT /incidents/<pk> * Tidy up tests * Update incident lead via API * /incidents list should be sorted by report date * Return primary key in incident response * Change deprecated parameter * Add docs for incidents endpoint
This refreshes the API for managing incidents, allowing list (/incidents), detail (/incidents/) and updating (PUT /incidents/<pk).