karrot-backend icon indicating copy to clipboard operation
karrot-backend copied to clipboard

Add activity roles

Open nicksellen opened this issue 4 years ago • 1 comments

Forum thread: https://community.foodsaving.world/t/applicant-trial-pickup-proposal/575 Frontend PR: https://github.com/yunity/karrot-frontend/pull/2421

This is a continuation of https://github.com/yunity/karrot-backend/pull/1105 but it took a slightly different path.

Previously:

  • activities could function just as normal
  • optionally you could require a certain role to join
  • and if you did that, you could optionally also allow any group members to join in "open" slots

Now:

  • there are participant types
  • each declares a name, required role, max_participants, and a description
  • it must always be declared explicitly now
  • there can be any number of them participant types per activity, or series
  • there is a new "member" role that all group members will always have (enforced by db constraint)

The same:

  • there is no extra roles, or mechanism to get them (see https://github.com/yunity/karrot-backend/pull/1167 for that work)
  • roles are still not customizable, that comes in the future

TODO:

  • [x] add API for updating participant roles for activity
  • [x] add API for updating participant roles for activity series
  • [x] handle updating activities with a series when changing participant roles
  • [ ] send notification if user removed from activity due to role requirement change?
  • [ ] rework notifications (push, reminder, emails) to be role aware
  • [x] make is_modified available in API (for UI)
  • [x] add tests for participant role API
  • [ ] deal with the TODO items in the code I left...
  • [x] remove max_participants / require_role / max_open_participants from models/API/etc

nicksellen avatar Nov 25 '21 13:11 nicksellen

Codecov Report

Merging #1199 (9374537) into master (32bd4d6) will decrease coverage by 0.33%. The diff coverage is 85.58%.

@@            Coverage Diff             @@
##           master    #1199      +/-   ##
==========================================
- Coverage   93.47%   93.13%   -0.34%     
==========================================
  Files         506      516      +10     
  Lines       19623    20298     +675     
  Branches     2245     2376     +131     
==========================================
+ Hits        18342    18904     +562     
- Misses       1012     1104      +92     
- Partials      269      290      +21     
Impacted Files Coverage Δ
karrot/activities/permissions.py 84.84% <0.00%> (-0.87%) :arrow_down:
karrot/conversations/api.py 94.14% <ø> (ø)
karrot/issues/factories.py 100.00% <ø> (ø)
karrot/issues/tests/test_api.py 99.34% <ø> (ø)
karrot/notifications/api.py 98.14% <ø> (ø)
karrot/template_previews/views.py 33.78% <15.00%> (-1.77%) :arrow_down:
karrot/activities/api.py 91.32% <17.64%> (-8.04%) :arrow_down:
karrot/activities/stats.py 96.55% <50.00%> (-3.45%) :arrow_down:
...ot/groups/migrations/0044_add_group_member_role.py 66.66% <66.66%> (ø)
karrot/utils/frontend_urls.py 83.17% <66.66%> (-0.64%) :arrow_down:
... and 47 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Nov 25 '21 13:11 codecov[bot]

I'll have a look at fixing the tests, then I could look into making existing notifications role-aware.

tiltec avatar Sep 10 '22 18:09 tiltec

I think the existing notifications should already be role aware. Although I could have missed some ..

I'm sure it could be improved too,

-------- Original Message -------- On 10 Sep 2022, 19:30, Tilmann Becker wrote:

I'll have a look at fixing the tests, then I could look into making existing notifications role-aware.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

nicksellen avatar Sep 10 '22 22:09 nicksellen

Yeah, notification were alright! I just saw the unticked checkbox in the PR description, so I thought this might be a nice small task :)

tiltec avatar Sep 11 '22 07:09 tiltec

I didn't find tests for modifying a series when an individual ~~pickup~~ activity was changed - from the code it looks like it's possible, but I didn't try it out. What's the intended behavior?

yes, should update everything that wasn't changed in the individual activities, including nested participant type fields, e.g.:

  • if you change the description in a participant type for a activity that is part of a series...
  • ... then modify the role for the participant type in that series
  • ... the change to role will be made to the activity that was edited, and the description will keep it's edited value

... I originally wasn't going to make it do such fiddly changes, and make it cruder, but went the whole way in the end.

the frontend understands this too, e.g. here the role has been modified from the series value for that particular participant type:

... this is possible because participant types are created per-activity, but still referenced back to the series participant type that they came from.

yes, more tests there would be good, my energy for this PR is going down a lot though! would :heart: to merge it soon!

... still waiting on a few changes to how the wording/notifications when participants are removed is done, so not quite ready.

nicksellen avatar Sep 14 '22 12:09 nicksellen