Skip to content
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.

Sign up for GitHub

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

Add Location For Library Books #3709

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Add Location For Library Books #3709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
41815b9
crecated table
srivastava-jyoti Sep 17, 2024
30f061c
created model
srivastava-jyoti Sep 17, 2024
5fba081
wip
srivastava-jyoti Sep 21, 2024
1ccf77d
wip
srivastava-jyoti Sep 21, 2024
d7d5d7d
added copies
srivastava-jyoti Oct 2, 2024
dfdd162
wip
srivastava-jyoti Oct 2, 2024
33db6cf
wip
srivastava-jyoti Oct 3, 2024
9d4f77e
updated copies
srivastava-jyoti Oct 7, 2024
261bb11
wip
srivastava-jyoti Oct 7, 2024
ab7ea3e
wip
srivastava-jyoti Oct 8, 2024
946411a
added copies
srivastava-jyoti Oct 9, 2024
b671887
wip
srivastava-jyoti Oct 9, 2024
b4cb784
wip
srivastava-jyoti Oct 9, 2024
c97e18a
wip
srivastava-jyoti Oct 11, 2024
35673d8
wip
srivastava-jyoti Oct 15, 2024
3f2ec3b
cleanups
srivastava-jyoti Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Modules/Operations/Entities/OfficeLocation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Modules\Operations\Entities;

use App\Models\KnowledgeCafe\Library\BookLocation;
use Illuminate\Database\Eloquent\Model;
use Modules\User\Entities\User;

Expand All @@ -13,4 +14,9 @@ public function centreHead()
{
return $this->belongsTo(User::class, 'centre_head_id');
}

public function locationOfBook()
{
return $this->hasMany(BookLocation::class, 'office_location_id');
}
}
74 changes: 63 additions & 11 deletions app/Http/Controllers/KnowledgeCafe/Library/BookController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
namespace App\Http\Controllers\KnowledgeCafe\Library;

use App\Http\Controllers\Controller;
use Illuminate\Support\Facades\DB;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor to use Eloquent models instead of the DB facade

Using the DB facade directly can bypass Eloquent's ORM benefits such as relationships, events, and attribute casting. Consider using Eloquent models for database operations to maintain consistency and improve code readability.

use App\Http\Requests\KnowledgeCafe\Library\BookRequest;
use App\Models\KnowledgeCafe\Library\Book;
use App\Models\KnowledgeCafe\Library\BookAMonth;
use App\Models\KnowledgeCafe\Library\BookCategory;
use App\Models\KnowledgeCafe\Library\BookLocation;
use Modules\Operations\Entities\OfficeLocation;
use App\Services\BookServices;
use Carbon\Carbon;
use Illuminate\Http\Request;
Expand All @@ -29,7 +32,7 @@ public function index(Request $request)
$searchCategory = $request->category_name ?? false;
$searchString = request()->has('search') ? request()->input('search') : false;
$categories = BookCategory::orderBy('name')->get();

$bookLocations = BookLocation::withTrashed()->with('location')->get();
switch (request()) {
case request()->has('wishlist'):
$books = auth()->user()->booksInWishlist;
Expand All @@ -48,9 +51,18 @@ public function index(Request $request)
$books->load('borrowers');
$wishlistedBooksCount = auth()->user()->booksInWishlist->count();
$booksBorrowedCount = auth()->user()->booksBorrower->count();

return view('knowledgecafe.library.books.index', compact('books', 'loggedInUser', 'categories', 'wishlistedBooksCount', 'booksBorrowedCount'));
}
$officeLocations = $this->officeLocationData();

$books = $books->map(function ($book) use ($bookLocations) {
$locations = $bookLocations->where('book_id', $book->id);
$copies = [];
foreach ($locations as $location) {
$copies[$location->office_location_id] = $location->number_of_copies;
}
$book->copies = $copies;
return $book;
});
Comment on lines +56 to +64
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Move data manipulation logic to a model or service class

Processing data within the controller can make the code less readable and harder to maintain. Consider moving the logic that maps books and attaches copies to a method within the Book model or a dedicated service class to adhere to the Single Responsibility Principle.

return view('knowledgecafe.library.books.index', compact('books', 'loggedInUser', 'categories', 'wishlistedBooksCount', 'booksBorrowedCount', 'bookLocations', 'officeLocations')); }

/**
* Show the form for creating a new resource.
Expand All @@ -59,9 +71,12 @@ public function index(Request $request)
*/
public function create()
{
return view('knowledgecafe.library.books.create');
return view('knowledgecafe.library.books.create', [
'officeLocations' => $this->officeLocationData(),
]);
}


/**
* Store a newly created resource in storage.
*
Expand All @@ -71,9 +86,13 @@ public function store(BookRequest $request)
{
$validatedData = $request->validated();
$ISBN = $validatedData['isbn'] ?? null;
Book::firstOrCreate(['isbn' => $ISBN], $validatedData);

return response()->json(['error' => false]);
$copiesLocation = $validatedData['copies'];
$book = Book::firstOrCreate(['isbn' => $ISBN], $validatedData);
$this->storeBookLocationWise($book->id, $copiesLocation);
return response()->json([
'error' => false,
'book_id' => $book->id
]);
Comment on lines +89 to +95
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate copies data and prevent mass assignment vulnerabilities

Using firstOrCreate with $validatedData may inadvertently allow mass assignment of unintended attributes. Ensure that only fillable attributes are mass assignable, and validate that copies is appropriately handled, especially if it's not a field on the Book model.

}

/**
Expand Down Expand Up @@ -106,10 +125,10 @@ public function show(Book $book)
public function update(BookRequest $request, Book $book)
{
$validatedData = $request->validated();
if (isset($validatedData['number_of_copies'])) {
$book->number_of_copies = $validatedData['number_of_copies'];
if (isset($validatedData['copies'])) {
$copiesLocation = $request['copies'];
$book->save();

$this->updatedBookLocationWise($book->id, $copiesLocation);
return response()->json(['isUpdated' => $book]);
}
if (isset($validatedData['categories'])) {
Expand Down Expand Up @@ -342,4 +361,37 @@ public function bookAMonthIndex()
'booksBorrowedCount' => $booksBorrowedCount,
]);
}

public function storeBookLocationWise($bookId, $copiesLocation)
{
$locationCopiesData = $copiesLocation;
foreach ($locationCopiesData as $locationId => $copies) {
BookLocation::create([
'book_id' => $bookId,
'office_location_id' => $locationId,
'number_of_copies' => $copies
]);
}
}
Comment on lines +365 to +375
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent duplicate BookLocation entries

The storeBookLocationWise method creates new BookLocation records without checking for existing entries, which can lead to duplicates. Consider using updateOrCreate to check for existing records and update them accordingly.


public function officeLocationData()
{
$officeLocations = OfficeLocation::all();
$officeLocationsData = $officeLocations->map(function ($officeLocation) {
return [
'center_id' => $officeLocation->id,
'centre_name' => $officeLocation->centre_name,
];
});
return $officeLocationsData;
}

public function updatedBookLocationWise($bookId, $copiesLocation) {
foreach ($copiesLocation as $locationId => $copies) {
DB::table('library_book_location')
->where('book_id', $bookId)
->where('office_location_id', $locationId)
->update(['number_of_copies' => $copies]);
}
}
Comment on lines +389 to +396
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle non-existent records in updatedBookLocationWise method

The updatedBookLocationWise method assumes records exist for each book_id and office_location_id combination. If no record exists, the update operation will have no effect. Use updateOrCreate or check for existence before updating to ensure all copies are correctly recorded.

}
1 change: 1 addition & 0 deletions app/Http/Requests/KnowledgeCafe/Library/BookRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public function rules()
'self_link' => 'nullable|string',
'number_of_copies' => 'nullable|integer|gte:1',
'on_kindle' => 'nullable',
'copies' => 'nullable',
];
}
}
6 changes: 6 additions & 0 deletions app/Models/KnowledgeCafe/Library/Book.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;
use Modules\User\Entities\User;
use App\Models\KnowledgeCafe\Library\BookLocation;

class Book extends Model
{
Expand Down Expand Up @@ -174,4 +175,9 @@ public function bookAMonths()
{
return $this->hasMany(BookAMonth::class, 'library_book_id');
}

public function bookLocations()
{
return $this->hasMany(BookLocation::class, 'book_id');
}
Comment on lines +179 to +182
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Incorrect relationship definition in BookLocation model

The relationship is incorrectly defined in the BookLocation model. It's using hasOne() when it should be using belongsTo() since BookLocation belongs to a Book. This violates the Laravel relationship best practices and could lead to unexpected behavior.

  • app/Models/KnowledgeCafe/Library/BookLocation.php: Change hasOne() to belongsTo() in the book() relationship method
     public function book()
     {
-        return $this->hasOne(Book::class, 'id', 'book_id');
+        return $this->belongsTo(Book::class, 'book_id');
     }
🔗 Analysis chain

Add PHPDoc for the relationship method

Please add PHPDoc to document the relationship and return type.

+    /**
+     * Get the locations associated with the book.
+     *
+     * @return \Illuminate\Database\Eloquent\Relations\HasMany
+     */
     public function bookLocations()
     {
         return $this->hasMany(BookLocation::class, 'book_id');
     }

Verify the inverse relationship

The relationship looks correct, but let's verify the inverse relationship in BookLocation model.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if BookLocation model has the inverse belongsTo relationship
ast-grep --pattern 'class BookLocation extends Model {
  $$$
  public function book() {
    $$$
    belongsTo($$$Book::class$$$)
    $$$
  }
  $$$
}'

Length of output: 151

Script:

#!/bin/bash
# Let's try a different approach to find the BookLocation model and check its relationships
# First, find the BookLocation model file
fd "BookLocation.php" --type f

# Then let's examine its content
rg "class BookLocation" -A 20 -B 5

Length of output: 1855

}
26 changes: 26 additions & 0 deletions app/Models/KnowledgeCafe/Library/BookLocation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace App\Models\KnowledgeCafe\Library;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;
use Modules\Operations\Entities\OfficeLocation;

class BookLocation extends Model
{
use SoftDeletes;

protected $table = 'library_book_location';
protected $fillable = ['book_id', 'office_location_id', 'number_of_copies'];
protected $dates = ['deleted_at'];

public function book()
{
return $this->hasOne(Book::class, 'id', 'book_id');
}
Comment on lines +17 to +20
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect relationship type used.

The hasOne relationship suggests that each BookLocation has one Book, but logically it should be the other way around - a BookLocation belongs to a Book.

Apply this fix:

 public function book()
 {
-    return $this->hasOne(Book::class, 'id', 'book_id');
+    return $this->belongsTo(Book::class);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function book()
{
return $this->hasOne(Book::class, 'id', 'book_id');
}
public function book()
{
return $this->belongsTo(Book::class);
}


public function location()
{
return $this->hasOne(OfficeLocation::class, 'id', 'office_location_id');
}
Comment on lines +22 to +25
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect relationship type used.

Similar to the book relationship, the location relationship should use belongsTo instead of hasOne.

Apply this fix:

 public function location()
 {
-    return $this->hasOne(OfficeLocation::class, 'id', 'office_location_id');
+    return $this->belongsTo(OfficeLocation::class, 'office_location_id');
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function location()
{
return $this->hasOne(OfficeLocation::class, 'id', 'office_location_id');
}
public function location()
{
return $this->belongsTo(OfficeLocation::class, 'office_location_id');
}

}
39 changes: 39 additions & 0 deletions database/migrations/2024_09_17_125527_create_library_book_location_copies_table.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

class CreateLibraryBookLocationCopiesTable extends Migration
{
/**
* Run the migrations.
*
* @return void
*/
public function up()
{
Schema::create('library_book_location', function (Blueprint $table) {
$table->id();
$table->unsignedInteger('book_id');
$table->unsignedBigInteger('office_location_id');
$table->integer('number_of_copies');
$table->timestamps();
});
Comment on lines +16 to +22
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add unique constraint and validation for number_of_copies.

  1. A unique constraint on (book_id, office_location_id) is needed to prevent duplicate entries.
  2. The number_of_copies column should be unsigned to prevent negative values.

Consider this improved schema:

 Schema::create('library_book_location', function (Blueprint $table) {
     $table->id();
     $table->unsignedInteger('book_id');
     $table->unsignedBigInteger('office_location_id');
-    $table->integer('number_of_copies');
+    $table->unsignedInteger('number_of_copies');
     $table->timestamps();
+    $table->unique(['book_id', 'office_location_id']);
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Schema::create('library_book_location', function (Blueprint $table) {
$table->id();
$table->unsignedInteger('book_id');
$table->unsignedBigInteger('office_location_id');
$table->integer('number_of_copies');
$table->timestamps();
});
Schema::create('library_book_location', function (Blueprint $table) {
$table->id();
$table->unsignedInteger('book_id');
$table->unsignedBigInteger('office_location_id');
$table->unsignedInteger('number_of_copies');
$table->timestamps();
$table->unique(['book_id', 'office_location_id']);
});


Schema::table('library_book_location', function (Blueprint $table) {
$table->foreign('book_id')->references('id')->on('library_books');
$table->foreign('office_location_id')->references('id')->on('office_locations');
});
}

/**
* Reverse the migrations.
*
* @return void
*/
public function down()
{
Schema::dropIfExists('library_book_location');
}
}
51 changes: 36 additions & 15 deletions resources/js/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ if (document.getElementById("show_and_save_book")) {
addMethod: "from_isbn",
showInfo: false,
book: {},
number_of_copies: 1,
copies: {},
routes: {
index: document.getElementById("show_book").dataset.indexRoute || "",
fetch: document.getElementById("book_form").dataset.actionRoute || "",
Expand Down Expand Up @@ -814,6 +814,7 @@ if (document.getElementById("show_and_save_book")) {
this.book["on_kindle"] = document.getElementById("on_kindle").checked
? 1
: 0;
this.$set(this.book, 'copies', this.copies);
axios.post(this.routes.store, this.book).then((response) => {
this.buttons.disableSaveButton = false;

Expand All @@ -836,6 +837,9 @@ if (document.getElementById("books_listing")) {
books: document.getElementById("books_table").dataset.books
? JSON.parse(document.getElementById("books_table").dataset.books)
: {},
bookLocations: document.getElementById("books_table").dataset.locations
? JSON.parse(document.getElementById("books_table").dataset.locations)
: [],
bookCategories: document.getElementById("books_table").dataset.categories
? JSON.parse(document.getElementById("books_table").dataset.categories)
: [],
Expand All @@ -857,6 +861,7 @@ if (document.getElementById("books_listing")) {
sortKeys: document.getElementById("category_input")
? document.getElementById("category_input").dataset.value
: "",
copies:{},
},
methods: {
updateCategoryMode: function(index) {
Expand Down Expand Up @@ -939,20 +944,28 @@ if (document.getElementById("books_listing")) {
return str.length > length ? str.substring(0, length) + "..." : str;
},

updateCopiesCount: function(index) {
var new_count = parseInt(
prompt(
"Number of copies of this book",
this.books[index].number_of_copies
)
);
if (new_count && isFinite(new_count)) {
this.books[index].number_of_copies = new_count;
axios.put(this.updateRoute + "/" + this.books[index].id, {
number_of_copies: new_count,
});
}
},
getTotalCopies: function(bookId) {
let totalCopies = 0;
this.bookLocations.forEach((location) => {
if (location.book_id === bookId) {
totalCopies += location.number_of_copies;
}
});
return totalCopies;
},
updateCopiesCount(index) {
let updatedCopies = this.books[index].copies;
axios.put(`${this.updateRoute}/${this.books[index].id}`, {
copies: updatedCopies,
})
.then(response => {
console.log("Copies updated successfully", response);
this.$set(this.books, index, { ...this.books[index], copies: updatedCopies });
})
.catch(error => {
console.error("Error updating copies", error);
});
}
},

mounted: function() {
Expand All @@ -965,6 +978,14 @@ if (document.getElementById("books_listing")) {
allCategoryInputs.forEach(
(checkbox) => (this.categoryInputs[checkbox.value] = checkbox)
);
this.books.forEach(book => {
this.$set(this.copies, book.id, {});
if (book.copies) {
Object.keys(book.copies).forEach(locationId => {
this.$set(this.copies[book.id], locationId, book.copies[locationId]);
});
}
});
},
});
}
Expand Down
Loading
Loading