Skip to content

Fix incorrect and failing reservation rule lists #756

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

Merged
merged 1 commit into from
May 3, 2025

Conversation

ddabble
Copy link
Member

@ddabble ddabble commented Mar 23, 2025

Proposed changes

APIReservationRuleListView inherited by mistake from MachineRelatedViewMixin instead of MachineTypeRelatedViewMixin, and the rest of the code was working with the assumption that the URL path's <int:pk> segment should be the PK of a machine type, like:

$.get(`${window.location.origin}/api/reservation/machinetypes/${this.machineType}/reservationrules/`, {}, (data) => {

So sometimes, the machine type PK in the URL happened to equal the PK of an existing machine, and other times, it did not. This fixes that :)

(Fun fact: This fix was actually applied locally on the server a couple years ago, and has been running there ever since 😅)

Areas to review closely

None in particular.

Checklist

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

  • Made sure the base for the PR is dev and not main
  • 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

...instead of machine type.
The rest of the code was working with the assumption that the URL path's
`<int:pk>` segment should be the PK of a machine type, like https://github.com/MAKENTNU/web/blob/2024-05-10/src/make_queue/static/make_queue/js/calendar.js#L393
@make-bot make-bot bot added this to web Mar 23, 2025
@make-bot make-bot bot moved this to Ready for Review in web Mar 23, 2025
Copy link

codecov bot commented Mar 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.17%. Comparing base (5715a77) to head (1d4aef5).
Report is 3 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #756   +/-   ##
=======================================
  Coverage   88.17%   88.17%           
=======================================
  Files         152      152           
  Lines        6198     6198           
=======================================
  Hits         5465     5465           
  Misses        733      733           
Files with missing lines Coverage Δ
src/make_queue/api/views.py 96.87% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ddabble ddabble mentioned this pull request Mar 23, 2025
10 tasks
@ddabble ddabble added the ⚡Quick review⚡ This PR is short and can be reviewed quickly. label Mar 23, 2025
@Gunvor4 Gunvor4 merged commit 36e489e into dev May 3, 2025
14 checks passed
@Gunvor4 Gunvor4 deleted the fix-reservation-rule-list-view-based-on-machine branch May 3, 2025 10:34
@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
⚡Quick review⚡ This PR is short and can be reviewed quickly.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants