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

Vouchers for user promotion - Part 2: Redemption #671

Merged
merged 205 commits into from
Nov 10, 2024

Conversation

fosterfarrell9
Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 commented Aug 13, 2024

This PR continues #670. Vouchers can be redeemed in order to take over tutorials, become a lecture editor, become a teacher or become a speaker (of a talk in a seminar).

To Dos

  • Redemption of tutor vouchers
  • Redemption of editor vouchers
  • Redemption of teacher vouchers
  • Redemption of speaker vouchers
  • Notifications for the redemption of tutor vouchers, editor vouchers, teacher vouchers and speaker vouchers
  • Write unit tests
  • Write cypress tests

Further To Dos (for reviewer)

  • Moving people around in lecture tutorials, should still be visible in dropdowns -> this is already covered by the existing tests
  • Test user deletion with redemptions -> should be deleted as well
  • Make sure secure hash also works with empty spaces at the beginning / at the end (when copied since oftentimes whitespaces are also copied)
  • Tell browser that secure hash text box shouldn't be auto-completable as to avoid storing these hashes as user suggestions.
  • Test sending of mails
  • Refactor VoucherProcessor
  • Refactor Lecture model
  • "Such a voucher [teacher voucher] will be invalidated directly after its redemption." -> make sure this is tested

Out of scope

Notes for the reviewer

The redemption of Vouchers is basically done via two actions of the VouchersController, namely #verify and #redeem.
In order to keep the controller lean, I introduced a service model VoucherProcessor in app/services (see e.g. here). Also, I extracted some stuff into the Notifier concern in apps/controllers/concerns since this contains some code also used by the LecturesController. On the model side, two new ActiveRecord models are used to store redemptions of vouchers: Redemption and Claim. The first one is basically a join table between User and Voucher (it stores that a certain user redeemed a certain voucher at a given time). The second one is a (polymorphic) join table between Redemption and so called Claimables, where the latter ones can be tutorials or talks. A claim describes what was taken over ("claimed") in the act of redeeming a voucher. In order to understand things it is probably best to play around a little in the GUI (with two users, one who is creating vouchers and one who is redeeming them) and observe what happens in the database. There are four different kinds of vouchers, hence there should be a lot to play around with. Depending on what kind of voucher that is redeemed, notifications in MaMpf and by email are issued:
For each kind of voucher, notifications in MaMpf are created for the teacher and editors of the lecture. Additionally,

  • for the redemption of a teacher voucher, one email is sent to the new teacher and one to the previous teacher informing them of the change
  • for the redemption of an editor voucher, an email is sent to the new editor
  • for the redemption of a speaker voucher, an email is sent to all cospeakers of the same talk informing them that another user has joined them

Note that redemptions only work additively: E.g. you can only take over tutorials, not resign from ones you are already a tutor of; similarly for the other kinds of vouchers. This is deliberate and by design: If you want to resign from a tutorial as a tutor you should contact a lecture editor. The same goes for the other kinds of status upgrades provided by vouchers.
I wrote unit tests for the vouchers model, the voucher processor and some security relevant routes in the vouchers controller. I have not yet written unit tests for some newly defined methods in the lecture model.

This is due to the form not being completely loaded while we
already go on.
Copy link
Member

@Splines Splines left a comment

Choose a reason for hiding this comment

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

Finally, after quite some time, I approve this PR. With this, we have a really nice voucher system at MaMpf, thank you 🎉

Please make sure to review the changes I've made before squash-merging. Follow-ups to this PR will be a redesign of the profile page where we can make the voucher redemption more prominent. I will also add a warning box to dropdowns that don't allow non-admin users to search for arbitrary users anymore. Just to not confuse them when no results are shown in the dropdown search box.

app/views/tutorials/_form.html.erb Outdated Show resolved Hide resolved
app/views/lectures/_new.html.erb Outdated Show resolved Hide resolved
app/controllers/lectures_controller.rb Show resolved Hide resolved
app/models/lecture.rb Outdated Show resolved Hide resolved
@fosterfarrell9 fosterfarrell9 merged commit 7ba50e8 into dev Nov 10, 2024
4 checks passed
@fosterfarrell9 fosterfarrell9 deleted the feature/voucher-redemption branch November 10, 2024 17:38
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.

2 participants