Skip to content

Conversation

CHSten
Copy link

@CHSten CHSten commented May 9, 2025

No description provided.

Copy link
Member

@krestenlaust krestenlaust left a comment

Choose a reason for hiding this comment

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

I will need a bit more time to do a proper review, and test it properly

But my first thoughts:

  • Having a few tests to check that the achievements keep working would be nice
  • You can delete all the new migrations and regenerate a single migration

Copy link
Member

@krestenlaust krestenlaust left a comment

Choose a reason for hiding this comment

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

Another partial review,

I've just tested it as a user, my immediate thoughts

As a member:

  • It's thrilling to have achievements, awesome-ly rewarding experience

  • The popup could be bigger to make it more noticeable (and more rewarding)

  • The popup should also last longer (maybe double the duration as now)

  • Given that the duration is long enough, you could also have a link to the userpage if you click it

  • As I understand it, the Top score right now is just the amount of achievements unlocked, but it would be cool to see individual achievement scores to understand the rarity of an achievement (this could be added later)

  • We also discussed possibility of (non-)hidden achievements (but this could always also be added later)

As an admin (without reading the guide):

  • The relationship between models is quite confusing, why have achievements be attached to a constraint, when constraints intuitively are attached to achievements. This would also allow constraint reuse, if you have multiple achievements for Halloween or similar. This is also true for tasks, if they should be reusable as well

  • Furthermore, naming a constraint/task to make it more reuseable could also be neat (or at the very least adding a note field)

  • Use specialized fields instead of generic text fields, an example of this is the achievement image field, you can't know what the root path is, or the format of paths. To improve this I suggest using FileField or similar (as seen in Kiosk), this also allows dynamic upload of achievement icons (then you can move the achievement images from the static folder into the fixture folder)

  • Add descriptions somewhere that explains what the fields in task do, I don't really understand task type, or the alcohol/caffeiene content fields

Overall, awesome work, if any of this doesn't make sense we can also discuss in person

Copy link

@henneboy henneboy left a comment

Choose a reason for hiding this comment

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

I have a few suggestions :)

@krestenlaust krestenlaust requested review from henneboy and removed request for ThomasBow and henneboy September 8, 2025 17:13
@krestenlaust
Copy link
Member

oops

@ThomasBow
Copy link
Contributor

ThomasBow commented Sep 30, 2025

Koden er meget fin, den har jeg ingen ændringer til. Dog da jeg testede det på min egen computer virker det til alle altid har top 100% i achievements generelt og i de individuelle
EDIT (ENGLISH VERSION):
The code lgtm, allthough when tested on my machine the top percentages in generel and individual achievements always showed as 100% for all users.

@CHSten
Copy link
Author

CHSten commented Oct 2, 2025

Koden er meget fin, den har jeg ingen ændringer til. Dog da jeg testede det på min egen computer virker det til alle altid har top 100% i achievements generelt og i de individuelle EDIT (ENGLISH VERSION): The code lgtm, allthough when tested on my machine the top percentages in generel and individual achievements always showed as 100% for all users.

@ThomasBow Jeg kunne ikke genskabe problemet. For mig ser det ud til at virke, medmindre jeg overser noget.
Koden fungerer sådan, at top-procenter (100%, 50% osv.) kun beregnes ud fra brugere, der allerede har opnået mindst én achievement. Fx hvis user0 har 3 og user1 har 2, så er user0 i top 50%, mens user1 stadig er i top 100%, da de ikke har flere end andre med achievements. Det samme gælder individuelt, men her ser man kun på hvor mange der har fået den specifikke achievement.

ENG:
I couldn’t reproduce the problem. For me, it seems to work unless I’m overlooking something. The code works so that top percentages (100%, 50%, etc.) are only calculated based on users who have already achieved at least one achievement. For example, if user0 has 3 and user1 has 2, then user0 is in the top 50%, while user1 is still in the top 100%, since they don’t have more than others with achievements. The same applies individually, but here it only looks at how many have obtained that specific achievement.

@ThomasBow
Copy link
Contributor

ThomasBow commented Oct 3, 2025

I can no longer recreate the problem on my machine either, so I guess everything looks good to me. Also I did not think about the total being calculated only from users who already have at least one achievement, though I do think it is a good idea 👍
Here are some receipts for it working for me now:
image
image

Copy link
Member

@krestenlaust krestenlaust left a comment

Choose a reason for hiding this comment

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

I think it's almost ready, I haven't tested it locally yet, I might not get time.

One of the things I'd like to test is how many more database queries a purchase causes with this change, vs. without.

I think Thomas also tested it locally.

The final thing I'd like, is at least ~10 suggestions for achievements to implement from day one when it's merged into the stregsystem (we can discuss in the FIT server) 💯

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to have achievement images in the repository? Or is this just for the fixture?

actions = [toggle_active_selected_products]


class AchievementForm(forms.ModelForm):
Copy link
Member

Choose a reason for hiding this comment

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

All this functionality looks like something that should be extracted to a different file, it looks like general IO for retrieving a particular filetype in the existing file structure. But I'm not even entirely sure what it does. Why not just have a file upload field, like in kiosk?

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need to update all the fixtures because of the migration, but I'll test it soon, and maybe just put this in the existing fixture, so we don't have to load all kinds of different fixtures

Copy link
Member

Choose a reason for hiding this comment

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

Lets add creation date and last updated fields to all new models

created_at = models.DateTimeField(auto_now_add=True)
updated_at = models.DateTimeField(auto_now=True)

return " | ".join(str_list)


class AchievementConstraint(models.Model):
Copy link
Member

Choose a reason for hiding this comment

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

Is there really no existing month or weekday widgets? It would be cool to extract the functionality as a custom standalone field

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.

4 participants