Skip to content

Usage requirements refactor#749

Merged
TheStrgamer merged 49 commits intodevfrom
course-refactor
May 3, 2025
Merged

Usage requirements refactor#749
TheStrgamer merged 49 commits intodevfrom
course-refactor

Conversation

@TheStrgamer
Copy link
Member

@TheStrgamer TheStrgamer commented Jan 30, 2025

Proposed changes

Machine types and courses use a new permission class instead of boolean for usage requirements

Areas to review closely

Checklist

(If any of the points are not relevant, mark them as checked)

  • Remembered to run the makemigrations, makemessages and compilemessages management commands and committed any changes that should be included in this PR
  • Created tests that fail without the changes, if relevant/possible
  • Manually tested that the website UI works as intended with different device layouts
    • (Most common is to test with typical screen sizes for mobile (320-425 px), tablet (768 px) and desktop (1024+ px), which can easily be done with your browser's dev tools)
  • Manually tested that everything works as intended when logged in as different users locally
    • (This can be e.g. anonymous users (i.e. not being logged in), "normal" non-member users, members of different committees, and superusers)
  • Made sure that your code conforms to the code style guides
    • (It's not intended that you read through this whole document, but that you get yourself an overview over its contents, and that you use it as a reference guide / checklist while taking a second look at your code before opening a pull request)
  • Attempted to minimize the number of common code smells
    • (See the comment for the previous checkbox)
  • Added sufficient documentation - e.g. as comments, docstrings or in the README, if suitable
  • Added your changes to the "Unreleased" section of the changelog - mainly the changes that are of particular interest to users and/or developers, if any
  • Added a "Deployment notes" section above, if anything out of the ordinary should be done when deploying these changes to the server
  • Structured your commits reasonably

ddabble and others added 30 commits October 21, 2021 13:49
Always use the pull request template
@Gunvor4 Gunvor4 requested a review from elisakiv February 20, 2025 19:01
@Gunvor4 Gunvor4 changed the base branch from main to dev February 20, 2025 19:38
Copy link
Contributor

@elisakiv elisakiv left a comment

Choose a reason for hiding this comment

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

Other than my singular comment, I can't seem to find any bugs or issues when running the code locally and looking at all the things I think will be impacted by the changes.

Maybe have a look at the failing check by codeclimate for some code cleanup stuff while you are changing things, it seems to mostly be about too many or not enough blank lines and similar nitpick.

Well done with the PR!

Copy link
Member

@ddabble ddabble left a comment

Choose a reason for hiding this comment

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

(Finally found some free time to review this 😅)

Very well done! 😄 I have some improvement suggestions below, but I'm impressed by your seemingly solid grasp of Django! 😊

@ddabble
Copy link
Member

ddabble commented Mar 23, 2025

Btw, it looks like you based this branch on main instead of dev, judging by the long list of merge commits at the beginning of this PR 😅 If you find yourself in a similar situation in the future, you can fix that by simply rebasing this branch on dev, followed by force-pushing the branch (if you've already pushed it, of course) - but for this PR, since we've already started to review it, it's usually pretty disruptive to force-push, as it will make it harder to spot any new changes (especially if you're rebasing on a branch that has different contents compared to the original base branch), so no worries :)

But we should definitely squash-merge the PR, unless you're planning on cleaning up the commit history before merging.

Copy link
Contributor

@Gunvor4 Gunvor4 left a comment

Choose a reason for hiding this comment

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

Good work! 👍 I don't have a lot of comments regarding this PR. But when I was reviewing, I realized that this would have been a much easier job if there were more comments in the code, so I would like to request some comments / explanations in the code. The make_queue app contains maybe the most difficult code on the entire page, so especially when we make changes to this app, we should make an effort to make this part of the page more understandable for future members of the dev committee.
A more thorough description of the PR could also have been helpful.

Copy link
Contributor

@Gunvor4 Gunvor4 left a comment

Choose a reason for hiding this comment

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

There are some problems with course registrations, both with the form itself, and with the behaviour of the init method on the Printer3DCourseForm. These have been there before, so they were not introduced by this PR, but they should nonetheless be fixed since we are changing this part of the page anyway.
First of all - it makes very little sense to have fields for both username and user, especially since it is actually possible to enter different user names in these two fields. The 'username' field is coupled to the 'full name' field, while the 'user' field suggests users from the User model while typing. This is a nice feature that should be kept in order to minimize the change for typos when entering usernames. Therefore, I would suggest removing the 'username' field, and auto-filling the 'full name' field from the 'user' field instead.
Also - if a user is already added to the User model with an EM number, the EM number will be erased from the User model when the same user is added to the Printer3DCourse model without an EM number. It is possible that the EM number will be restored by information received from Feide, but new instances to the Printer3DCourse model should still not be able to delete information from the User model, so this is obviously a bug, and it should be fixed. If an EM number is already present for the user that is being added to the Printer3DCourse model - it should instead be auto-filled in the EM number field, like the user's full name is in the 'full name' field.

@TheStrgamer TheStrgamer merged commit a744023 into dev May 3, 2025
12 checks passed
@TheStrgamer TheStrgamer deleted the course-refactor branch May 3, 2025 10:46
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in web May 3, 2025
@TheStrgamer TheStrgamer mentioned this pull request May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New functionality on the page

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants