Cl/owner return #245
Cl/owner return #245
Conversation
|
@DefineDan mir ist aufgefallen, das ich evtl. etwas falsch verstanden habe. In der jetzigen Implementierung gibt der Owner quasi das Item zurück, weil er ja auf Rückgabe drückt. Ist das so gewollt oder soll der user eingetragen werden, der das Item persönlich zurückgegeben hat? Das könnte sich etwas schwieriger gestalten und wie sollte dann der Owner den User eintragen? |
@Tonybodo Vielleicht kurz zum Hintergrund: Es wird demnächst wahrscheinlich eine Ausleihhistorie implementiert, Da soll zu sehen sein, wer wann was ausgeliehen und zurückgegeben hat. Bisher war es immer so, dass der Borrower das Item zurückgegeben hat, damit ist es bei der Rückgabe eindeutig, wer das war. Wenn jetzt aber auch jeder Owner (also jeder mit can_manage) das Item zurückgeben kann, dann ist es wichtig zu tracken, wer das Item zurückgegeben hat. Also war das z.B. User1 (Owner/Manager), User2 (Owner/Manager) oder User3 (Borrower). Wir wollen also zu jeder Rückgabe speichern, welcher User das war (letztenendes wer den Rückgabe Button drückt). Falls das Datenmodell das zur Zeit nicht hergibt, dann lass uns nochmal überlegen, wie aufwendig das wäre und ob es das wert ist. |
|
So wie ich das jetzt verstehe soll aber der Item owner später in der Ausleihhistorie angezeigt werden richtig? Ich werde mich nochmal morgen mit den Leuten vom Datenmodell absprechen, denn ich denke, dass dieser Punkt dem Datenmodell noch hinzugefügt werden muss. |
|
Also wenn das Datenmodell dafür angepasst werden muss, dann würde ich das erstmal sein lassen. Aber wenn es mit dem jetzigen so geht, wie ich das beschrieben habe, dann sollten wir das einfach mitspeichern. Ich review später noch das UI. |
There was a problem hiding this comment.
All in all a great feature so far! I like the variety of the tests for the owner_can_return method, in my opinion all tests for this method are covered.
There are a few things that I would like to discuss or fix before merging, that includes changing the give_back method where the lending is completed and writing a test for it, maybe changing the owner_can_return method and changing a small typo in the test descriptions in features/items/show_spec.rb.
app/controllers/items_controller.rb
Outdated
| @@ -123,16 +123,20 @@ def give_back | |||
| @lending = Lending.where(item_id: @item.id, user_id: @user.id, completed_at: nil).first | |||
| @lending.completed_at = Time.current | |||
| msg = I18n.t("items.messages.successfully_returned") | |||
| elsif @item.borrowed? && @user.can_manage?(@item) | |||
| @lending = Lending.where(item_id: @item.id, user_id: @user.id, completed_at: nil).first | |||
There was a problem hiding this comment.
Do we need to filter for @user.id? Because then we get all Lendings for the current_user, but as the current_user is the owner, who is not necessary the current borrower, I think we are getting the wrong results out of this query.
I think we need a test in features/items that creates an owner_user and a borrower_user, where the owner has managing rights and the borrower not. There must exist a lending without completed_date for the borrower. Then we need to sign_in the owner and expect the page to have a "Return as owner` button, on which the owner clicks. We then must test, that there exists no lending anymore for the borrower.
app/models/item.rb
Outdated
| @@ -104,6 +104,10 @@ def borrowed_by?(user) | |||
| Lending.exists?(user_id: user.id, item_id: id, completed_at: nil) | |||
| end | |||
|
|
|||
| def owner_can_return?(user) | |||
| !borrowed_by?(user) && borrowed? && user.can_manage?(self) | |||
There was a problem hiding this comment.
Here I wouldnt pass the user to the method, but rather write !borrowed_by?(current_user) && borrowed? && current_user.can_manage?(self), because now reading the method looks a bit weird.
Maybe we can also think about implementing this method on the user. It could then look like this:
def can_return_as_owner?(item)
!item.borrowed_by(self) && item.borrowed? && can_manage?(item)
end
And call the method like so: some_user.can_return_as_owner?(item)
| format.html { redirect_to @item, notice: msg } | ||
| format.json { render :index, status: :ok, location: @item } | ||
| else | ||
| format.html { render :show, status: :unprocessable_entity, notice: msg } | ||
| format.html { render @item, status: :unprocessable_entity, notice: msg } |
There was a problem hiding this comment.
Just out of curiosity, what was the reason for this change?
spec/features/items/show_spec.rb
Outdated
| @@ -145,4 +145,19 @@ | |||
| expect(page).not_to have_text(:visible, I18n.t("items.buttons.delete")) | |||
| expect(page).not_to have_text(:visible, I18n.t("items.buttons.download_qrcode")) | |||
| end | |||
|
|
|||
| it "not display owner-return button if item is not borrowed or borrowed by the owner himself" do | |||
There was a problem hiding this comment.
Please change the title to `it "does not....."
Furthermore, is the second part of the test description necessary ("or borrowed by the owner himself")? Because there is another test down below, which tests exactly this behaviour.
spec/features/items/show_spec.rb
Outdated
| expect(page).not_to have_text(:visible, I18n.t("items.buttons.owner-return")) | ||
| end | ||
|
|
||
| it "not display owner-return button if item is borrowed by the owner himself" do |
There was a problem hiding this comment.
Please change the title to `it "does not....."
Um den Button zu sehen, muss man borrowed? auf true setzen, oder man testet es mit 2 Usern.