Skip to content
This repository has been archived by the owner on May 20, 2024. It is now read-only.

Add trust for role #1167

Merged
merged 31 commits into from
Sep 30, 2022
Merged

Add trust for role #1167

merged 31 commits into from
Sep 30, 2022

Conversation

nicksellen
Copy link
Member

@nicksellen nicksellen commented Jul 14, 2021

Forum thread: https://community.foodsaving.world/t/applicant-trial-pickup-proposal/575
Backend part for trust for role feature karrot-dev/karrot-frontend#2361

It:

  • adds role field to Trust model
  • accepts role field in the trust user / revoke trust API endpoints
  • returns all trust info in group membership details in trust field
  • adds a role model to let groups specify custom roles, with fields display_name, description, threshold, role_required_for_trust
  • adds notifications and email when somebody got a new role
  • works with existing frontend by:
    • preserving trusted_by serializer field in group membership info
    • defaulting to 'editor' if not specified

To try it out, use the database shell to create a new Role for a given group:

Group.objects.get(id=1).roles.create(
  name='approved', 
  display_name='Bestätigt', 
  description='Wenn man bestätigt ist, kann man tollere Abholungen machen.'
  role_required_for_trust='editor',
)

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #1167 (2ab9274) into master (88a6b06) will increase coverage by 0.02%.
The diff coverage is 97.20%.

@@            Coverage Diff             @@
##           master    #1167      +/-   ##
==========================================
+ Coverage   93.04%   93.07%   +0.02%     
==========================================
  Files         532      533       +1     
  Lines       20526    20658     +132     
  Branches     2415     2437      +22     
==========================================
+ Hits        19099    19227     +128     
- Misses       1132     1133       +1     
- Partials      295      298       +3     
Impacted Files Coverage Δ
karrot/groups/serializers.py 89.77% <92.50%> (+0.53%) ⬆️
karrot/groups/receivers.py 87.64% <94.28%> (+0.97%) ⬆️
karrot/groups/api.py 97.82% <100.00%> (ø)
karrot/groups/emails.py 100.00% <100.00%> (ø)
...arrot/groups/migrations/0049_auto_20220930_1506.py 100.00% <100.00%> (ø)
karrot/groups/models.py 96.96% <100.00%> (+0.17%) ⬆️
karrot/groups/stats.py 96.22% <100.00%> (+0.30%) ⬆️
karrot/groups/tests/test_api.py 100.00% <100.00%> (ø)
karrot/groups/tests/test_serializers.py 100.00% <100.00%> (ø)
karrot/groups/tests/test_trust.py 100.00% <100.00%> (ø)
... and 3 more

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

@tiltec tiltec marked this pull request as ready for review September 22, 2022 20:11
@tiltec
Copy link
Member

tiltec commented Sep 22, 2022

Next, I'll add the bit to actually assign the role when there's at least one trust. And to remove it again when trust is revoked and there's less than one trust left.... I guess that can somehow work...

@nicksellen
Copy link
Member Author

Next, I'll add the bit to actually assign the role when there's at least one trust. And to remove it again when trust is revoked and there's less than one trust left.... I guess that can somehow work...

Great :) Makes sense for me to review it after that.

The notifications might be the more fiddly, I think the ones for getting editor role are very specific to that, but would be useful if they were generalizable, so when roles are customizable per group there is just one set of role/trust notifications.

@tiltec
Copy link
Member

tiltec commented Sep 23, 2022

I wondered if it could make sense to have those notifications somehow user-defined; after all, I have no idea what the role is actually good for. From my perspective, it's just a lower-case string ;)

We probably need those messages:

  • human-readable role name
  • frontend trust dialog, info about role
  • frontend you_got_role notification text (could also be used for backend email)
  • frontend role_removed notification text
  • ...

This would turn Karrot more into a Content Management System, but could be a useful level of customization. I guess groups would write better messages than we could, because they are closer to the users.

This also resolves the question about translation, groups would provide the messages in their own language.

@nicksellen
Copy link
Member Author

I wondered if it could make sense to have those notifications somehow user-defined; after all, I have no idea what the role is actually good for. From my perspective, it's just a lower-case string ;)

Totally :) That's exactly the kind of direction that makes sense to me too. And more generally having more and more bits of the app that the groups themselves have written. This is part of the same vision of the customizable activity types, place types, etc...

I think there is an intermediate step though, as that's a lot of work to get to that, and it would probably be best to do some design process first to understand the context for the groups better.

So, to my mind a way to notify about people getting approved role is enough for now, whether that's by generalizing the existing editor content, or making new content is up to you :) (or someone else if you aren't doing that bit)

@tiltec
Copy link
Member

tiltec commented Sep 23, 2022

I mean, I wouldn't implement any API method to change the text yet, but still have it in the database so we can adapt to the Luxembourg group needs without re-deploying (but just change the database entry).

Basically, my idea is to set up the code so the Luxembourg group can try out their "approved" role with text of their own choosing (set through us), and if other groups would like to join the beta test, they can do so with fairly low effort for us (just changing a few database fields).

I wouldn't touch the existing messages for editor yet, would leave that for later.

At some point, we can add the API to add roles and change messages. Then it makes sense to adjust the editor messages, as roles would become more prevalent in the groups.

Copy link
Member Author

@nicksellen nicksellen left a comment

Choose a reason for hiding this comment

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

Great, I think it's amazing to progress this stuff.

It does feel an intermediate state now, not on your part, but because we're bolting concepts on to our existing system, which I think would benefit from some rationalization, which I think we are capable to manage now! So great to work with what we have right now.

I can't actually approve this, because GitHub tells me it's my PR, because I did the initial work on it :)

karrot/groups/serializers.py Show resolved Hide resolved
karrot/groups/templates/user_got_role.mjml Outdated Show resolved Hide resolved
karrot/groups/tests/test_serializers.py Outdated Show resolved Hide resolved
karrot/groups/serializers.py Outdated Show resolved Hide resolved
@nicksellen
Copy link
Member Author

I wouldn't touch the existing messages for editor yet, would leave that for later.

yup, makes sense!

@nicksellen nicksellen merged commit fb0784f into master Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants