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

Feedback for finished pickups #253

Closed
tiltec opened this Issue Feb 23, 2017 · 22 comments

Comments

Projects
None yet
5 participants
@tiltec
Member

tiltec commented Feb 23, 2017

For the frontend feature yunity/karrot-frontend#159 we need a backend implementation.

I imagine two database fields per collector (user who did the pickup):

  • comment: text (for future reference)
  • weight: kilograms (for statistical purpose)

These information should be stored together with the pickup. Currently, the pickups (upcoming and finished) are all in one model, but I want to move the finished pickups to a new history model:

An instance of PICKUP_DONE looks like this:

  {  
    "id": "197",
    "date": "2017-02-22T14:59:22.442526Z",
    "typus": "PICKUP_DONE",
    "group": "5",
    "store": "22",
    "users": [
      "94",
      "90",
      "92",
      "100",
      "108"
    ],
    "payload": {
      "series": "22",
      "max_collectors": "10"
    }
  }

I propose to add the data to the payload field, which is a schema-less Postgres JSONField.

As each collector can give one feedback, we could have a user_feedback object with each user id as key.

...
"payload": {
  "series": "22",
  "max_collectors": "10",
  "user_feedback": {
    "94": {
      "comment": "There were many soft tomatos. Better bring some boxes next time.",
      "weight": "5"
    },
    "108": {
      "comment": "I can recommend this store. Nice and friendly!",
      "weight": "3"
    }
  }
}

What do you think about this data structure? Does it make sense to you?

Next steps are: design of API endpoints and serializer.

@Sliverriver

This comment has been minimized.

Sliverriver commented Mar 6, 2017

is it just to have one feedback per user ? or also have multiple comments ?

@tiltec

This comment has been minimized.

Member

tiltec commented Mar 6, 2017

One feedback. What is the use case for multiple ones? I think if it needs to be updated, we can leave the field editable for some days after the feedback was given.

@tiltec

This comment has been minimized.

Member

tiltec commented May 7, 2017

@mddemarie Can you estimate when you will have time to work on this task?
Bruno raised focus on this feature here: yunity/karrot-frontend#158

@mddemarie

This comment has been minimized.

Contributor

mddemarie commented May 25, 2017

@tiltec I am sorry that I did not answer before.
I tried to work on the issue today, but I have problems with my setup. I will try it again in the next days, but I am not sure if I will be successful soon. If you need it soon, I will need some help – or is it better for you if I give the issue back?
In some weeks I will have a lot more time (and help) – maybe you saw it in Slack?

@tiltec

This comment has been minimized.

Member

tiltec commented Jun 30, 2017

This is ready to work on, feel free to ask me any questions :)

@tiltec

This comment has been minimized.

Member

tiltec commented Jul 27, 2017

Given my latest insights in yunity/karrot-frontend#159 (comment) and the recent Slack discussion, my data structure proposal from above seems like a bad choice:

  • it requires modification of history events after they have been created (to add the feedback)
  • it would make the history entry the original data source of the feedback. Whereas in all other cases, the history entry just stores metadata about other models
  • it requires complex database queries to fulfil the frontend needs: find out which users can give feedback, list all feedback per store, calculate statistics

Therefore, I propose: add a new Feedback model into the store's models.py. It should reference the user giving the feedback and the pick-up the feedback is about. This makes it easy to list the feedback per store and to calculate statistics.

class Feedback(BaseModel):
  given_by = # reference to user model
  about = # reference to pickup date model
  weight = # positive number that will get interpreted as kilograms
  comment = # arbitrary text

The following API structure can be derived from it:

POST /api/feedback/: create a new feedback
GET /api/feedback/?store=5: list all feedback for store with ID 5

This should address (2) and (3) of the frontend requirements: yunity/karrot-frontend#159 (comment)

To make the reference to the pickup date model work, I will open another issue to make pickup date entries persistent (i.e. don't delete them when they are over).

@nicksellen

This comment has been minimized.

Member

nicksellen commented Jul 27, 2017

I sat with @id-gue and @mddemarie - we went through all the initial conversations about what the feature was for (in slack, and the frontend/backend issues) to get a clearer understanding of why the feedback is useful.

We then explored some possible technical solutions of how to implement it, and discussed the pros/cons of each of them. Here are the notes I made whilst we chatted:

Option 0 - current code

  • when PickupDate is complete
  • delete PickupDate (via command run once a minute)
  • create PICKUP_DONE history entry

Option 1 - CHANGE THE PAST!

  • when PickupDate is complete
  • delete PickupDate (via command run once a minute)
  • create PICKUP_DONE history entry
  • add feedback (comment and weight per user) in PICKUP_DONE history entry

comments:

  • if user gives feedback before history entry is created, there is nowhere to store it
  • involves editing history (just feels wrong! you cannot change the past)

Option 2 - new Feedback model (Tilmann idea)

  • never delete PickupDate entries (just filter for future or past ones depending on needs)
  • probably still add PICKUP_DONE history entry when pickup is done
  • new model Feedback (1 for each user/pickup combination) to collect comment and weight

comments:

  • @mddemarie wonders: what is it good for? feels heavy
  • @id-gue wonders: perhaps could be made into a more generic feedback/commentable model?
  • simple because it is just creating normal models like the other ones (add your stuff in api.py/serializers.py/models.py etc...)

Option 3 - store feedback on PickupDate model

  • add weight and comment fields on PickupDate model
  • use a JSONField to allow feedback for each user that did the pickup
  • ... would also require not deleting old PickupDate items
  • ... would probably also create PICKUP_DONE entry (to show in history tab...)
  • very simliar to option 2, but using a JSONField instead of a seperate Feedback model

The current work has gone in this direction and can be seen here:
mddemarie@85e79e6?diff=split

comments:

  • would be easier to implement for Ines/Marie as they already know mostly how to do it, option 2 feels like it would require a bunch more learning

Option 4 - 24 hour feedback window

  • keep PickupDate entry for 24 hours after it is complete
  • allow feedback, and store in JSONField
  • similar to option 1, then delete the PickupDate, and add a PICKUP_DONE history entry

comments:

  • 24 hour window seems a good idea
  • seems odd to implement time window in a technical way (storing in PickupDate table before, and history after)
@nicksellen

This comment has been minimized.

Member

nicksellen commented Jul 27, 2017

would be grateful for your feedback @tiltec

@id-gue was quite into option 2, and @mddemarie is warming up to it :)

@tiltec

This comment has been minimized.

Member

tiltec commented Jul 27, 2017

@id-gue

This comment has been minimized.

Contributor

id-gue commented Jul 28, 2017

@tiltec so we will work on option 2! :)

I think it might be the best to remove our code changes and start from scratch, right? The only thing we could commit now is the new permission class IsPast, right?

Everything else should be refactored. Hope, we will be faster this time... ;-)

@tiltec

This comment has been minimized.

Member

tiltec commented Aug 4, 2017

create a new feedback

POST /api/feedback/

with data

{
    "about": 33,
    "weight": 2,
    "comment": "string"
}

get a list of all feedbacks:

GET /api/feedback/

should return

[
  {
    "id": 1,
    "given_by": 33,
    "about": 33,
    "weight": 2,
    "comment": "string"
  },
  {
    "id": 2,
    "given_by": 34,
    "about": 33,
    "weight": 2,
    "comment": "string"
  }
]

and so on.

Edge cases to consider

What happens when a user is trying to

  • give a feedback for a pickup that he didn't do (= is not part of the collectors)?
  • give a feedback on a pickup that is not done yet (date is not in the past)?
  • get a feedback for a pickup in a store in a group when he is not a member of the group? (should not be found)

There's surely more that I didn't think of yet :) Makes sense to create test cases and then improve the code, as I mentioned in our call.

@id-gue

This comment has been minimized.

Contributor

id-gue commented Aug 8, 2017

@mddemarie

This comment has been minimized.

Contributor

mddemarie commented Aug 22, 2017

@tiltec About who GETs the Feedback (just to be sure):

non-user gets 400
user gets 400
group member gets 200

Right?

(And wouldn't the Feedback be interesting for users-but-not-group-members, too?)

@tiltec

This comment has been minimized.

Member

tiltec commented Aug 22, 2017

Yes, that seems correct (user means non-group-member). Although it should be 404 ("Not found") instead of 400 ("Bad request") in case of GET.

(And wouldn't the Feedback be interesting for users-but-not-group-members, too?)

I don't think so. I think that the groups are generally private/closed and don't want to have internals of their pick-ups leaking to non-group-members or non-users.

@id-gue

This comment has been minimized.

Contributor

id-gue commented Aug 22, 2017

Ok, great. Thank you!

tiltec added a commit that referenced this issue Aug 31, 2017

WIP: User Feedback for PickUps Tests (#342)
Related issue: #253 

* added the correct API endpoint feedback

* added the correct API endpoint feedback

* corrected API endpoint feedback

* Flaked some files

* changes with flake8

* cleaned up stored.py

* minor changes in url routers

* playing with FeedbackSerializer and FeedbackModel

* deleted the signal post_feedback_create from signals.py and comment it out in serializers.py as well as some flake8 corrections

* upload the changes in signals.py (which should habe been in the previous commit).

* fixing bugs

* commented out the fields given_by and about in the feedback model for testing purpose

* created a first test case, started with another test if the user is not a member of group

* added a new test for which has to be written code in serializers.py

* added user validation for feedback serializer

* removed unneeded code from the serializer and added comments to the tests

* added a new test and changed the Feedback validator

* bug fixing in test_feedback and playing with FeedbackSerializer

* added 1 positive test for displaying feedback as user + uncommented non-working code

* flake8

* added migration

* flake8 corrections

* fixed 1 test with failing non-group member and extended validate_about with code for non-group member

* added test that fails if you are not assigned to the pickup

* corrected the code in serializers for Feedback, validate_about

* minor changes in FeedbackSerializer

* fixed the Feedback api test (members / users)

* added permission IsMember to FeedbackViewSet and three tests for the method GET (one fails at the moment)

* experimented with FeedbackViewSet, tried to raise Http404 error if the user is non-member of group, see comments

* turned one failing into passing test and added one more test + more comments in apy.py

* added tilmanns code to the serializer

* added Feedback.objects.create() to SetUpClass in test_feedback_api.py

* added queryset function to api to fix the FeedbackTests for list

* more tests for getting single feedback with GET.id for non-user, user, group member and collector

* 1 more test for the feedback in the future + comment clean-up

* test the feedback model and changed the parameters of the fields weight and comment

* removed comments and all unnecessary code

* test if feedback can be created if comment to long
@tiltec

This comment has been minimized.

Member

tiltec commented Aug 31, 2017

I continued working on the beta-frontend for giving feedback and discovered that I can submit multiple feedback for the same pickup. Maybe worth a fix :)

@id-gue

This comment has been minimized.

Contributor

id-gue commented Sep 1, 2017

Hi @tiltec so there are 3 things we should do:

  1. add filters (by group, store and pickup-date)
  2. limit multiple feedback for the same pickup per collector
  3. make sure that weight=null AND comment=null fails

Optional:

  • refactoring about
  • editing given feedback
  • at the moment every collector can give a weight estimation - do we want an average weight? If yes: does that require additional backend functionality?
@tiltec

This comment has been minimized.

Member

tiltec commented Sep 1, 2017

Yes, sounds good!

editing given feedback

That would make users happy, I think.
Perhaps we should limit the overall period in which feedback can be given and edited to, say, 1 month after it has been done?

at the moment every collector can give a weight estimation - do we want an average weight? If yes: does that require additional backend functionality?

This would be a nice follow-up task: creating statistics. Maybe we schedule a call before you start working on that?
Related issues: yunity/karrot-frontend#355 yunity/karrot-frontend#159

@tiltec

This comment has been minimized.

Member

tiltec commented Sep 1, 2017

Currently, weight is a PositiveIntegerField, but I think we should also allow fractions of kilograms. Maybe the FloatField is a better choice? It would need checking for values smaller than zero though.

@mddemarie

This comment has been minimized.

Contributor

mddemarie commented Sep 4, 2017

Thank you for your reply.
We make a plan for adding more functionality to Feedback tomorrow.

@tiltec

This comment has been minimized.

Member

tiltec commented Sep 5, 2017

Hey, just saw it in your dailylog and have feedback regarding it:

  • Make sure that weight=null AND comment=null fails
  • Change ‘weight’ field to FloatField
  • Add PATCH
  • Refacture the field about
  • Add filters
  • Limit multiple feedback for the same pickup per collector
  • Limit the overall period in which feedback can be given

As filters are pretty important for the frontend, I would pull them into (1).
I think "Limit multiple feedback for the same pickup per collector" is also quite important to avoid inconsistencies.

Adding PATCH needs a bit more fiddling (e.g. make sure that the about field is not changed), so I would move it to (2).

Also, I'd suggest to make small Pull Request for each change, this way we can integrate them faster.

So my suggestion would be to reorder them by type of change and priorities. No new task added :)

  1. Fixes
  • Limit multiple feedback for the same pickup per collector
  • Make sure that weight=null AND comment=null fails
  • Change ‘weight’ field to FloatField
  1. Necessary functionality
  • Add filters
  1. Niceties
  • Add PATCH
  • Refactor the field about
  • Limit the overall period in which feedback can be given
  1. New feature
  • Statistics

Of course, if you are more motivated to work on one of the "further ahead" tasks, that's totally fine with me too!

@tiltec

This comment has been minimized.

Member

tiltec commented Nov 5, 2017

This seems to be mostly done, we should now implement it in the frontend and get user feedback!

@tiltec tiltec closed this Nov 5, 2017

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