Add a new method to remove an entry #31
Conversation
Adds ability for logged-in users to delete entries with confirmation dialog. Includes updated UI, database query changes, and comprehensive test coverage.
|
Resolves #28 |
|
To provide feedback, navigate to the Files changed tab and leave comments on the proposed code changes. Choose Start review for each comment, and then choose Request changes, and I'll propose revised changes. |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
| if not session.get('logged_in'): | ||
| abort(401) | ||
| db = get_db() | ||
| db.execute('DELETE FROM entries WHERE id = ?', [entry_id]) |
There was a problem hiding this comment.
Warning
Description: The delete_entry function lacks error handling for database operations. Add a try-except block around the database operations to handle potential SQLite errors.
Severity: High
There was a problem hiding this comment.
The fix addresses the lack of error handling in the delete_entry function by adding a try-except block around the database operations. This modification catches potential SQLite errors, rolls back the transaction if an error occurs, and provides appropriate feedback to the user. The fix improves the robustness of the code by handling database errors gracefully and preventing potential crashes or data inconsistencies.
| db.execute('DELETE FROM entries WHERE id = ?', [entry_id]) | |
| if not session.get('logged_in'): | |
| abort(401) | |
| db = get_db() | |
| try: | |
| db.execute('DELETE FROM entries WHERE id = ?', [entry_id]) | |
| db.commit() | |
| flash('Entry was successfully deleted') | |
| except sqlite3.Error as e: | |
| db.rollback() | |
| flash(f'Error deleting entry: {str(e)}') | |
| return redirect(url_for('show_entries')) |
| def show_entries(): | ||
| db = get_db() | ||
| cur = db.execute('SELECT title, text FROM entries ORDER BY id DESC') | ||
| cur = db.execute('SELECT id, title, text FROM entries ORDER BY id DESC') |
There was a problem hiding this comment.
Description: The SQL query in show_entries() fetches all entries without pagination, which may lead to performance issues with large datasets. Consider implementing pagination for the entries query to limit the number of results returned.
Severity: Medium
There was a problem hiding this comment.
The fix implements pagination for the entries query in the show_entries() function. It limits the number of results returned by using LIMIT and OFFSET in the SQL query, based on the page number requested. This addresses the performance issue with large datasets by fetching only a subset of entries at a time.
| cur = db.execute('SELECT id, title, text FROM entries ORDER BY id DESC') | |
| @app.route('/') | |
| def show_entries(): | |
| db = get_db() | |
| page = request.args.get('page', 1, type=int) | |
| per_page = 10 # Number of entries per page | |
| offset = (page - 1) * per_page | |
| cur = db.execute('SELECT id, title, text FROM entries ORDER BY id DESC LIMIT ? OFFSET ?', | |
| (per_page, offset)) | |
| entries = cur.fetchall() | |
| return render_template('show_entries.html', entries=entries, page=page) | |
| @app.route('/add', methods=['POST']) |
| g.sqlite_db.close() | ||
|
|
||
|
|
||
| @app.route('/') |
There was a problem hiding this comment.
Description: The show_entries() function mixes database operations with view logic, potentially reducing maintainability. Consider separating database operations into a dedicated function or class to improve code organization and maintainability.
Severity: Low
There was a problem hiding this comment.
The fix addresses the comment by separating the database operations from the view logic in the show_entries() function. A new function get_entries() is introduced to handle the database query, improving code organization and maintainability. The show_entries() function now calls get_entries() to retrieve the entries and passes them to the template rendering.
| @app.route('/') | |
| g.sqlite_db.close() | |
| def get_entries(): | |
| """Retrieves all entries from the database.""" | |
| db = get_db() | |
| cur = db.execute('SELECT id, title, text FROM entries ORDER BY id DESC') | |
| return cur.fetchall() | |
| @app.route('/') | |
| def show_entries(): | |
| entries = get_entries() | |
| return render_template('show_entries.html', entries=entries) |
| # Should get a 401 Unauthorized response | ||
| assert response.status_code == 401 | ||
|
|
||
| def test_delete_entry_authorized(self): |
There was a problem hiding this comment.
Description: The test_delete_entry_authorized function is quite long and performs multiple operations. Consider breaking down the test into smaller, focused test cases or helper methods.
Severity: Low
There was a problem hiding this comment.
Sorry, I'm not able to suggest a fix for this review finding.
Request ID : f95047e6-ed2b-45db-8527-d6581605bb4e
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
This pull request adds a delete functionality for entries in the Flask application. It includes:
The changes enhance the application's content management capabilities while maintaining proper security controls by restricting deletion to authenticated users.