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

Improve handling of pickup series modifications v2 #591

Merged
merged 49 commits into from Jan 1, 2019

Conversation

Projects
None yet
1 participant
@tiltec
Copy link
Member

tiltec commented Dec 11, 2018

Context: https://community.foodsaving.world/t/better-change-handling-for-recurring-pickups/174

#587 turned out to have some problems, here's the new approach: we always keep non-empty pickups, but they can be disabled individually.

  1. changing a recurring pickup keeps non-empty pickups
    a. if time, weekdays or any other part of the recurrence rule changes, create new pickups where necessary and delete empty non-matching pickups
    b. if the description of the recurring pickup changes, rewrite descriptions of its existing pickups
    c. if max slots of the recurring pickup changes, rewrite max slots of its existing pickups
  2. changing the "weeks in advance" setting of a store keeps non-empty pickups
  • Disabled pickups can be enabled again.
  • Pickups that are part of a series cannot be moved.
  • Series API returns dates that belong to the series (client can identify leftover pickups)

To do

  • move series updates from store serializer into receiver
  • fix commented out test in test_pickupdate_series_api
  • remove last_changed_by from API and other places (if not necessary)

Open questions:

  • how to migrate existing changed pickups?
  • email/push notifications about disabled pickups?
  • do anything on group timezone change?

tiltec added some commits Nov 17, 2018

simplify mark_as_read
the special case handling is not necessary when it only runs on creation
do not show cancelled pickups as done or missed, rename done_and_proc…
…essed into feedback_possible, add stats about cancelled pickups
rename pickup.deleted to pickup.is_cancelled, prevent deletion of pic…
…kups, allow to uncancel pickups, remove store.deleted
rename cancel -> disable, add more notifications, rename override_pic…
…kups -> update_pickups, remove add_new_pickups
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #591 into master will decrease coverage by 0.14%.
The diff coverage is 95.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
- Coverage   95.68%   95.53%   -0.15%     
==========================================
  Files         316      326      +10     
  Lines       11702    11919     +217     
  Branches      690      717      +27     
==========================================
+ Hits        11197    11387     +190     
- Misses        418      437      +19     
- Partials       87       95       +8
Impacted Files Coverage Δ
foodsaving/pickups/tests/test_pickupdates_api.py 100% <ø> (ø) ⬆️
foodsaving/history/utils.py 100% <ø> (ø) ⬆️
foodsaving/stores/tests/test_stores_api.py 100% <ø> (ø) ⬆️
...aving/pickups/tests/test_pickupdates_api_filter.py 100% <ø> (ø) ⬆️
foodsaving/utils/misc.py 100% <100%> (ø) ⬆️
...saving/pickups/tests/test_pickupdate_series_api.py 99.38% <100%> (-0.03%) ⬇️
foodsaving/notifications/models.py 98.03% <100%> (+0.89%) ⬆️
foodsaving/pickups/tests/test_models.py 100% <100%> (ø) ⬆️
...ving/history/migrations/0006_auto_20181208_2205.py 100% <100%> (ø)
...ving/pickups/migrations/0006_auto_20181207_2210.py 100% <100%> (ø)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df85a7...e227509. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #591 into master will increase coverage by 0.06%.
The diff coverage is 98.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
+ Coverage   95.68%   95.74%   +0.06%     
==========================================
  Files         316      322       +6     
  Lines       11702    12045     +343     
  Branches      690      719      +29     
==========================================
+ Hits        11197    11533     +336     
- Misses        418      423       +5     
- Partials       87       89       +2
Impacted Files Coverage Δ
foodsaving/notifications/stats.py 100% <ø> (ø) ⬆️
...aving/pickups/tests/test_pickupdates_api_filter.py 100% <ø> (ø) ⬆️
foodsaving/pickups/tests/test_pickupdates_api.py 100% <ø> (ø) ⬆️
foodsaving/history/utils.py 100% <ø> (ø) ⬆️
foodsaving/subscriptions/tests/test_consumers.py 38.53% <0%> (ø) ⬆️
foodsaving/pickups/stats.py 100% <100%> (ø) ⬆️
foodsaving/notifications/tests/test_receivers.py 100% <100%> (ø) ⬆️
...saving/pickups/tests/test_pickupdate_series_api.py 99.44% <100%> (+0.03%) ⬆️
...ving/history/migrations/0006_auto_20181216_2133.py 100% <100%> (ø)
foodsaving/history/tests/test_api.py 100% <100%> (ø) ⬆️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29200c5...38219e8. Read the comment docs.

@tiltec tiltec referenced this pull request Dec 11, 2018

Merged

Improve handling of pickup series modifications v2 #1147

3 of 10 tasks complete
@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Dec 16, 2018

I think I need to reimplement the feature that pickups can be moved because people use this regularly in production (karrot.world has 9 upcoming pickups that have been moved)

One approach would be to implement it like this:

  1. move pickup to new time
  2. remove pickup from series (series=None) -> makes sure the pickup doesn't get deleted automatically
  3. create new disabled pickup at the original time -> makes sure that the series won't create a non-disabled at the original time automatically again

But it has some drawbacks:

  • not possible to undo (can be moved back, but not be made part of the series again)
  • will create a disabled pickup that gets shown in the frontend (clutter...)
  • it might cause duplicates if the pickup is moved far to the future where another pickup will be created automatically

Another approach would be to store the original time (e.g. pickup.original_date). This field would only be used in the series.update_pickups function to match existing pickups. It should prevent the creation of a new pickup at the original time.
If we prevent duplicates, it might lead to funny behavior (e.g. automatic creation of pickups when pickup is moved a second time)...

@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Dec 17, 2018

Talked with @djahnie yesterday evening: seems to complex to properly implement moved recurring pickups, users can adapt. It's more important that the users can understand the behavior of the software.

I added a data migration that makes upcoming moved pickups into one-time pickups and it creates a dummy pickup as part of the series at the original time to prevent new pickups from being created automatically.

@tiltec tiltec merged commit 0a4218d into master Jan 1, 2019

5 checks passed

ci/circleci: test Your tests passed on CircleCI!
Details
codecov/patch 98.72% of diff hit (target 95.68%)
Details
codecov/project 95.74% (+0.06%) compared to 29200c5
Details
security/snyk - mjml/package.json (tiltec) No new issues
Details
security/snyk - requirements.txt (tiltec) No new issues
Details

@tiltec tiltec deleted the series-changes branch Jan 1, 2019

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