Add a new method to remove an entry #4
Conversation
Implements the ability for logged-in users to delete entries. Includes: - Delete endpoint with authentication check - UI button with confirmation dialog - Unit tests for authorized/unauthorized deletion
|
Resolves #3 |
|
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}
| 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 current page number and entries per page. 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']) |
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
| """ | ||
| Test that an unauthorized user cannot delete entries. | ||
| """ | ||
| with app.test_client() as client: |
There was a problem hiding this comment.
Description: The test functions could benefit from using pytest fixtures for setup and teardown. Consider using pytest fixtures to set up the test client, login state, and database entries.
Severity: Low
There was a problem hiding this comment.
The fix implements pytest fixtures for client, authentication, and database initialization. These fixtures improve test organization, reduce code duplication, and ensure proper setup and teardown for each test. The client fixture provides a test client, auth fixture handles authentication, and init_database fixture initializes and cleans up the database for each test. The test functions are updated to use these fixtures, improving their reliability and maintainability.
| with app.test_client() as client: | |
| import pytest | |
| @pytest.fixture | |
| def client(): | |
| return app.test_client() | |
| @pytest.fixture | |
| def auth(client): | |
| return AuthActions(client) | |
| @pytest.fixture | |
| def init_database(): | |
| with app.app_context(): | |
| init_db() | |
| yield | |
| # Clean up the database after the test | |
| db = get_db() | |
| db.execute('DELETE FROM entries') | |
| db.commit() | |
| def test_show_entries(client): | |
| """ | |
| Test the show_entries function to ensure it correctly renders the template | |
| with the entries from the database. | |
| """ | |
| # Make a GET request to the root URL | |
| response = client.get('/') | |
| # Check if the response status code is 200 (OK) | |
| assert response.status_code == 200 | |
| # Check if the response contains the expected content | |
| assert b'<title>Amazon Q Developer Flask Demo</title>' in response.data | |
| # Remove the check for <h2>Entries</h2> as it's not in the template | |
| # Note: We're not checking for specific entries here because | |
| # the database state is not guaranteed. In a real-world scenario, | |
| # you might want to set up a known database state before running this test. | |
| def test_delete_entry_unauthorized(client): | |
| """ | |
| Test that an unauthorized user cannot delete entries. | |
| """ | |
| # Try to delete an entry without being logged in | |
| response = client.post('/delete/1', follow_redirects=True) | |
| # Should get a 401 Unauthorized response | |
| assert response.status_code == 401 | |
| def test_delete_entry_authorized(client, auth, init_database): | |
| """ | |
| Test that an authorized user can delete entries. | |
| """ | |
| # First, log in | |
| auth.login() | |
| # Add an entry to delete | |
| client.post('/add', data={ | |
| 'title': 'Test Entry to Delete', |
This pull request adds the ability for logged-in users to delete entries in the Flask application. The changes include:
The update improves the application's content management capabilities while maintaining security through proper authentication checks.