Skip to content

Comments

feat(rpg): reveal found diamond list to diamond owner (fixes #307)#449

Open
pi625 wants to merge 3 commits intovEnhance:mainfrom
pi625:my-new-feature
Open

feat(rpg): reveal found diamond list to diamond owner (fixes #307)#449
pi625 wants to merge 3 commits intovEnhance:mainfrom
pi625:my-new-feature

Conversation

@pi625
Copy link

@pi625 pi625 commented Feb 17, 2026

  • Show 'found list' link on diamond solution page to the creator
  • Allow creator to access the FoundList view (previously staff-only)
    (finished both)

If you're an OTIS student, include your OTIS-WEB username or student ID number
(whichever you prefer) so I can grant you the spades bounty as well.

Student ID: 3064

…#307)

- Show 'found list' link on diamond solution page to the creator
- Allow creator to access the FoundList view (previously staff-only)
@coveralls
Copy link

coveralls commented Feb 17, 2026

Pull Request Test Coverage Report for Build 22127357045

Details

  • 47 of 47 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 93.577%

Totals Coverage Status
Change from base Build 22066968771: 0.03%
Covered Lines: 8566
Relevant Lines: 8979

💛 - Coveralls

@jatloe
Copy link
Contributor

jatloe commented Feb 17, 2026

uh oh

rpg/views.py Outdated
isinstance(self.request.user, User)
and self.request.user == achievement.creator
):
return super(StaffRequiredMixin, self).dispatch(*args, **kwargs)
Copy link
Owner

Choose a reason for hiding this comment

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

what is this line doing?

Copy link
Owner

@vEnhance vEnhance left a comment

Choose a reason for hiding this comment

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

There should be some unit tests to verify this does the correct thing.

I think some comment about why this would be desirable would also be helpful for context.

@pi625 pi625 closed this Feb 17, 2026
- Add test_found_list_creator_access to verify creators can see their diamond's found list
- Fix dispatch method to properly check authentication before bypassing staff check
- Add explanatory comments for the permission bypass logic
@pi625 pi625 reopened this Feb 17, 2026
@pi625 pi625 marked this pull request as draft February 17, 2026 22:54
@pi625 pi625 marked this pull request as ready for review February 18, 2026 05:11
@pi625
Copy link
Author

pi625 commented Feb 18, 2026

I added a unit test that passed and fixed dispatch logic to properly check authentication before bypassing staff check and commented a lot more. For the feature itself, I don't see it as significantly desirable or undesirable, just a quality of life type thing(and I wanted to practice coding in a way that wouldn't significantly affect anything/maybe be slightly beneficial). I added a main and edge case unit test and the code works with all other tests. happy to adjust further if needed!

@vEnhance
Copy link
Owner

I have some slight elegance concerns about hijacking dispatch like this, in that StaffRequiredMixin is now a lie. From a maintenance perspective it's undesirable for me that the signature of the view no longer reflects what it actually does. I think perhaps it would be better if we had a separate view for the creator of a diamond, though it could still use the same template.

BTW, are you using an LLM to write this? It's fine if so, but it's useful for me to know because the kind of things I need to look for in a review are different from AI-generated code versus human-written code. (I should probably mention this in contributing guidelines.)

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