Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #546 +/- ##
===========================================
- Coverage 100.00% 99.92% -0.08%
===========================================
Files 112 118 +6
Lines 2498 2716 +218
Branches 82 99 +17
===========================================
+ Hits 2498 2714 +216
- Misses 0 2 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
D0gmaDev
force-pushed
the
feat/rooms-model
branch
from
135c642 to
b6db6e4
Compare
D0gmaDev
force-pushed
the
feat/rooms-model
branch
from
4bf8a92 to
bc3244a
Compare
|
|
||
| validates :number, presence: true, uniqueness: true, length: { maximum: 6 }, | ||
| format: { with: /\A[A-Z0-9]+\z/, message: 'must be uppercase alphanumeric' } | ||
| validates :group, presence: true, length: { maximum: 6 }, |
There was a problem hiding this comment.
Maybe a comment/example to understand what a "group" is?
There was a problem hiding this comment.
Before I write it in the code, what do you think about:
- A room group represents the natural grouping of rooms. It can be the room number itself or a shared identifier
| format: { with: /\A[A-Z0-9]+\z/, message: 'must be uppercase alphanumeric' } | ||
| validates :group, presence: true, length: { maximum: 6 }, | ||
| format: { with: /\A[A-Z0-9]+\z/, message: 'must be uppercase alphanumeric' } | ||
| validates :building, presence: true, inclusion: { in: ('A'..'F').to_a } |
There was a problem hiding this comment.
Is "Foyer" in the F building? Accueil or Halle projet rooms in A?
| # frozen_string_literal: true | ||
|
|
||
| class Room < ApplicationRecord | ||
| has_one :user, foreign_key: :room, primary_key: :number, dependent: :restrict_with_error, inverse_of: :room_record |
There was a problem hiding this comment.
Still unsure about using a non-fixed column as a foreign key, all to avoid one join...
If we use the standard primary key, what do you think of reversing the has_one/belong_to association? Like we do with IPs, it would make finding free rooms easier.
| validates :room, uniqueness: true, allow_nil: true | ||
| validate :room_must_exist, if: -> { room.present? } |
There was a problem hiding this comment.
Using the primary key would also drop all this validation code
| ensure | ||
| Rails.logger = old_logger |
| test 'changing room enqueues room sync job' do | ||
| assert_enqueued_with(job: SyncRoomToSsoJob, args: [@user.id]) do | ||
| @user.update!(room: rooms(:room_b231).number) |
|
|
||
| test 'sync_room skips user without oidc_id' do | ||
| @user.oidc_id = nil | ||
| assert_nothing_raised do |
There was a problem hiding this comment.
Same here, you can remove the assert_nothing_raised (and add tests that check the actual behavior of the method if you want/can)
| ) | ||
| .to_return(status: 200, body: '{"setDate":"2026-01-01T00:00:00Z"}') | ||
|
|
||
| ProductionSsoService.new.sync_room(@user) |
There was a problem hiding this comment.
Another option to test with production? => true could be with a MiniTest stub:
require 'minitest/mock'
Rails.env.stub(:production?, true) do
SsoMetadataService.sync_room(@user)
end(But the mock functionality has been extracted from MiniTest 6.0 because they don't want to maintain it anymore, maybe the simple inheritance is OK)
| assert_requested(stub) | ||
| end | ||
|
|
||
| test 'sync_room does not raise on POST HTTP failure' do |
There was a problem hiding this comment.
Wait, we really want to have silent errors?
2 participants
Closes #544