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

Add api for history objects. #217

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Add api for history objects. #217

wants to merge 16 commits into from

Conversation

ataalik
Copy link
Contributor

@ataalik ataalik commented Feb 20, 2020

For #215
Adds api endpoints for inspecting changes on STVs and TEs.
You can filter STVs using these parameters:

  • "user" for user id
  • "stv" for stv id
  • "entity" for entity id that stvs belong to.
    You can filter TEs using:
  • "user" for user id
  • "entity" for entity id that stvs belong to.

Reverting changes can be done by sending a PUT request to a specific reversion instance. Ex: PUT /api/te-history/1/

There is also default DRF pagination using "limit" and "offset" parameters.

What is missing:

  • Reverting history with api. Done!
  • Ability to add reason for changes.

Copy link
Member

@quorth0n quorth0n left a comment

Choose a reason for hiding this comment

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

Looks good, could you add some short tests for the history endpoints?

@ataalik
Copy link
Contributor Author

ataalik commented Feb 28, 2020

@whirish I wrote some basic stuff. It failed normally because the mockup did not add user info about changes. I wonder how should we handle that in production. I just made it so it returned none now.

Also on stv tests there seems to be not many changes, is it because the order of the tests? How can I make sure it happens after certain changes? Or should I make the changes and test that they are there in the same method? That seems more reliable.

@quorth0n
Copy link
Member

quorth0n commented Mar 11, 2020

@ataalik Sorry for the late response, totally missed your commit.
When you say the mockup are you referring to the mi-auth branch on the frontend? Or are you generating this historical data through the admin panel or something similar. AFAIK if django auth has registered that the user is authenticated the historical changes should be performed automatically, but I haven't tested it outside of just the admin panel.
As far as your STV tests go, definitely try to make the changes within the same test method, if only that method will access that data. If multiple methods need to access it, look into generating it in an overriden setUpTestData method like some of the other tests do.

Copy link
Contributor

@MiklerGM MiklerGM left a comment

Choose a reason for hiding this comment

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

Looks cool! Thank you.

Btw, can we squash migrations?

project/api/views/endpoints/stv_downloader.py Outdated Show resolved Hide resolved
def get_queryset(self):
queryset = self.queryset
if self.action == "list":
stv = self.request.query_params.get("stv", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this return all events, even the creation event? Should we filter it out?

Copy link
Member

Choose a reason for hiding this comment

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

It is a subclass of ReadOnlyViewSet, so no creation method is implemented

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, what should we do with initial geometry upload event? Should it be accessible and shown to user?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, in case we want to rollback to the instance's initial state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes are logged in three categories. Creation, change, deletion. As Liam said creation is used for rolling back to initial version so it is needed I think.

@@ -168,3 +168,26 @@ def test_api_can_not_create_stv(self):
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
response = self.client.post(url, data_overlapping)
self.assertEqual(response.status_code, status.HTTP_409_CONFLICT)

@authorized
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need an authorization to perform a get request for stv-history-list?

Copy link
Member

Choose a reason for hiding this comment

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

After closer inspection yes we do, do you think an unauthorized user would have need to see historical records? If so we can change it

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it would be easier without extra headers on GET, because I already have all the coding done for this.
On the other side, I can't object against doing it as a protected with authorization method.

@MiklerGM
Copy link
Contributor

What will happened if you'll try to rollback not the latest change? Will it rewrite all the newer changes?

Example:

  1. Uploaded polygon A [geom_a_1]
  2. Uploaded polygon B, overlaps subtracted from A [geom_a_2, geom_b_1]
  3. Uploaded polygon C, overlaps subtracted from A and B [geom_a_3, geom_b_2, geom_c_1]
  4. Uploaded polygon D, overlaps subtracted from B and C [geom_a_3, geom_b_3, geom_c_2, geom_d_1]

What will happened if user will decide to revert history batch №3, what will remain in the database?

  • Copy from previous values [result: geom_a_2, geom_b_1, geom_d_1]
    Then overlap subtraction from geom_b in history batch was ignored and geom_a_2 was not checked for overlapping geom_d

  • Merging [ result: geom_a_4, geom_b_4, geom_d_1]
    DELETE geom_a_3 and geom_b_3 (current values in the database)
    ADD geom_a_2 and geom_b_1
    SUBTRACT everything from geom_a_2 and geom_b_1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants