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 participant types for activities #2421

Merged
merged 37 commits into from Sep 21, 2022
Merged

Conversation

nicksellen
Copy link
Member

@nicksellen nicksellen commented Jul 21, 2021

Overall issue: #2361
Forum thread: https://community.foodsaving.world/t/applicant-trial-pickup-proposal/575
Backend PR: karrot-dev/karrot-backend#1199

What does this PR do?

UI for specifying an activity that has a role requirement, and optionally a number of open participants.

participanttypes

It's nearly ready! A few remaining tasks:

  • remove "type name" field
  • change "require role" to "limit to"
  • display "member" role as "anyone"
  • add explanation of roles (member/anyone, editor, approved)
  • add warning when toggling off "Use participant types", that information might be lost
  • maybe some stuff needed when editing participant types in a way that will kick some people out? (+ a confirmation message)
  • add "newcomer" as actual role (more a backend task)
  • do something when checkSave fails (to try: put a negative number in max_participants)
  • write the rest of the remaining TODOs
  • do the remaining TODOs

Links to related issues

Checklist

  • added a test, or explain why one is not needed/possible...
  • no unrelated changes
  • asked someone for a code review
  • joined chat.foodsaving.world/channel/karrot-dev
  • added an entry to CHANGELOG.md (description, pull request link, username(s))
  • tried out on a mobile device (does everything work as expected on a smaller screen?)

@github-actions
Copy link

This pull request is marked as stale because it has not had any activity for 90 days.

It doesn't mean it's not important, so please remove the stale label if you like it, or add a comment saying what it means to you :)

However, if you just leave it like this, I'll close it in 7 days to help keep your pull requests tidy!

Thanks!

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #2421 (d00270c) into master (4f132e5) will decrease coverage by 0.50%.
The diff coverage is 84.13%.

@@            Coverage Diff             @@
##           master    #2421      +/-   ##
==========================================
- Coverage   77.26%   76.76%   -0.51%     
==========================================
  Files         356      359       +3     
  Lines       35949    37010    +1061     
  Branches     1776     1825      +49     
==========================================
+ Hits        27777    28410     +633     
- Misses       8172     8600     +428     
Impacted Files Coverage Δ
src/authuser/components/Login.vue 0.00% <0.00%> (ø)
src/authuser/components/PasswordReset.vue 0.00% <0.00%> (ø)
src/authuser/components/RequestPasswordReset.vue 0.00% <0.00%> (ø)
src/group/components/GroupEdit.vue 0.00% <0.00%> (ø)
src/history/components/HistoryEntry.vue 70.27% <0.00%> (ø)
src/messages/components/ConversationCompose.vue 92.07% <0.00%> (ø)
src/messages/components/DetailHeaderUI.vue 55.48% <0.00%> (ø)
src/places/components/PlaceEdit.vue 0.00% <0.00%> (ø)
src/places/pages/Layout.vue 93.24% <0.00%> (ø)
src/history/components/HistoryDetail.vue 67.94% <12.50%> (-3.49%) ⬇️
... and 33 more

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

@tiltec
Copy link
Member

tiltec commented Sep 10, 2022

Just got this to work locally, sharing some impressions:

Screen Shot 2022-09-10 at 13 57 03 Screen Shot 2022-09-10 at 13 57 10

Looks very nice :) I'll have a look if I can help, but I think there's not much to do before it can be merged. Probably gonna focus at the backend PR first.

Some (possibly unwanted) feedback that came to my mind when looking at the UI - no need to change anything!

  • old way of joining activities by clicking on slot doesn't work anymore, how do you feel about adding it back before a release?
  • this is the first time the word "editor" is more prominently used towards Karrot users, before it was always a bit more indirect, like "got editing permission" or "user with editing permission". could give a different impression, or be somehow confusing (like being a magazine editor)
  • "use participant types" in ActivitySeriesEdit could be "use multiple participant types"
  • "Participant type 1" headline in ActivitySeriesEdit might not be necessary at all

Copy link
Member

@tiltec tiltec left a comment

Choose a reason for hiding this comment

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

Phew, that was a ride! Pretty comprehensive PR, it seems to me - congrats for making it so far 😄

A lot of i18n missing, I left some comments.

I'll miss the feature to click a slot to join... but can do without.

There's an empty InvitationsForm.vue file that you added, probably in error.

src/activities/components/ActivityEdit.vue Outdated Show resolved Hide resolved
src/activities/components/ActivityItem.story.js Outdated Show resolved Hide resolved
src/activities/components/ActivityItem.vue Outdated Show resolved Hide resolved
src/activities/components/ActivityItem.vue Outdated Show resolved Hide resolved
src/activities/components/ActivitySeriesEdit.vue Outdated Show resolved Hide resolved
src/activities/components/ParticipantTypesEdit.vue Outdated Show resolved Hide resolved
src/activities/components/ParticipantTypesEdit.vue Outdated Show resolved Hide resolved
src/activities/components/ParticipantTypesEdit.vue Outdated Show resolved Hide resolved
src/utils/mixins/editMixin.js Show resolved Hide resolved
src/activities/components/ActivitySeriesEdit.vue Outdated Show resolved Hide resolved
@nicksellen
Copy link
Member Author

old way of joining activities by clicking on slot doesn't work anymore, how do you feel about adding it back before a release?

I would prefer to leave it out... on the basis that:

  • new way is more explicit, and I think clearer
  • could have both ways, would not be opposed, but also confusing to have 2 ways?
  • it simplified the famously complicated ActivityUsers.vue component by removing that :) (although I had made some improvements to that on another now-merged branch...)

maybe we leave it out for merge, and welcome to re-visit the topic before release?

this is the first time the word "editor" is more prominently used towards Karrot users, before it was always a bit more indirect, like "got editing permission" or "user with editing permission". could give a different impression, or be somehow confusing (like being a magazine editor)

indeed! roles become explicit.... this is part of the work towards group-defined roles, and other stuff like that, so I think it's a needed step to bring roles out into the light...

I think currently roles are confusing anyway, so maybe more explicit is helpful there?

@nicksellen nicksellen marked this pull request as ready for review September 15, 2022 15:41
@nicksellen nicksellen merged commit cf22b82 into master Sep 21, 2022
@nicksellen nicksellen changed the title Activities that require role Add participant types for activitie Sep 26, 2022
@nicksellen nicksellen changed the title Add participant types for activitie Add participant types for activities Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants