Add a new method to remove an entry #19
Conversation
Implements delete functionality for blog entries that can only be accessed by logged-in users. Includes UI changes to show delete buttons and new backend route with appropriate tests.
|
Resolves #13 |
|
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.
Warning
Description: The SQL query in show_entries() function is not using parameterized queries, which can lead to performance issues and potential SQL injection vulnerabilities. Use parameterized queries with db.execute() to improve performance and security.
Severity: High
There was a problem hiding this comment.
Sorry, I'm not able to suggest a fix for this review finding.
Request ID : 8a1b30fd-834b-4b60-85ce-99ead916d802
| 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 remove_entry function lacks error handling for database operations. Consider wrapping the database operations in a try-except block to handle potential SQLite errors.
Severity: High
There was a problem hiding this comment.
The fix addresses the lack of error handling in the remove_entry function by wrapping the database operations in a try-except block. 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.
| 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 removed') | |
| except sqlite3.Error as e: | |
| db.rollback() | |
| flash(f'Error removing entry: {str(e)}') | |
| return redirect(url_for('show_entries')) |
| # 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_remove_entry_unauthorized(self, client): |
There was a problem hiding this comment.
Description: The test function is quite long and performs multiple operations, which may reduce readability and maintainability. Consider breaking down the test into smaller, more focused test functions.
Severity: Low
There was a problem hiding this comment.
The fix addresses the comment by breaking down the long test_remove_entry_unauthorized function into smaller, more focused test functions. The original function has been split into three separate functions: test_remove_entry_unauthorized_setup, test_remove_entry_unauthorized_attempt, and test_remove_entry_unauthorized_verify. Additionally, a helper method get_entry_id has been introduced to reduce code duplication. This refactoring improves readability and maintainability of the test suite.
| def test_remove_entry_unauthorized(self, client): | |
| # 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_remove_entry_unauthorized_setup(self, client): | |
| """ | |
| Setup for testing unauthorized entry removal. | |
| """ | |
| client.post('/login', data={'username': 'admin', 'password': 'default'}) | |
| client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'}) | |
| client.get('/logout') | |
| return self.get_entry_id('Test Title') | |
| def test_remove_entry_unauthorized_attempt(self, client): | |
| """ | |
| Test that an unauthorized user cannot remove an entry. | |
| """ | |
| entry_id = self.test_remove_entry_unauthorized_setup(client) | |
| # Try to remove the entry without being logged in | |
| response = client.post(f'/remove/{entry_id}', follow_redirects=True) | |
| # Check that we get a 401 Unauthorized response | |
| assert response.status_code == 401 | |
| def test_remove_entry_unauthorized_verify(self, client): | |
| """ | |
| Verify that the entry still exists after unauthorized removal attempt. | |
| """ | |
| entry_id = self.test_remove_entry_unauthorized_setup(client) | |
| self.test_remove_entry_unauthorized_attempt(client) | |
| # Verify the entry still exists | |
| with app.app_context(): | |
| db = get_db() | |
| entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone() | |
| assert entry is not None | |
| assert entry['title'] == 'Test Title' | |
| def get_entry_id(self, title): | |
| """ | |
| Helper method to get the entry ID for a given title. | |
| """ | |
| with app.app_context(): | |
| db = get_db() | |
| return db.execute('SELECT id FROM entries WHERE title = ?', [title]).fetchone()['id'] | |
| def test_remove_entry_authorized(self, client): | |
| """ | |
| Test that an authorized user can remove an entry. | |
| """ | |
| # First add an entry as a logged-in user | |
| client.post('/login', data={'username': 'admin', 'password': 'default'}) | |
| client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'}) | |
| # Get the entry ID | |
| entry_id = self.get_entry_id('Test Title') | |
| # Remove the entry | |
| response = client.post(f'/remove/{entry_id}', follow_redirects=True) | |
| # Check that the removal was successful | |
| assert response.status_code == 200 |
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
| assert entry is not None | ||
| assert entry['title'] == 'Test Title' | ||
|
|
||
| def test_remove_entry_authorized(self, client): |
There was a problem hiding this comment.
Description: The test function is long and performs multiple operations, making it harder to understand and maintain. Consider breaking down the test into smaller, focused test functions for each operation (login, add entry, remove entry).
Severity: Low
There was a problem hiding this comment.
The fix addresses the comment by breaking down the long test_remove_entry_authorized function into three smaller, focused test functions: test_remove_entry_authorized_login, test_remove_entry_authorized_add, and test_remove_entry_authorized_remove. This improves readability and maintainability by separating the login, add entry, and remove entry operations into distinct test cases.
| def test_remove_entry_authorized(self, client): | |
| # Check that we get a 401 Unauthorized response | |
| assert response.status_code == 401 | |
| # Verify the entry still exists | |
| with app.app_context(): | |
| db = get_db() | |
| entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone() | |
| assert entry is not None | |
| assert entry['title'] == 'Test Title' | |
| def test_remove_entry_authorized_login(self, client): | |
| """ | |
| Test that an authorized user can log in. | |
| """ | |
| response = client.post('/login', data={'username': 'admin', 'password': 'default'}) | |
| assert response.status_code == 302 # Redirect after successful login | |
| def test_remove_entry_authorized_add(self, client): | |
| """ | |
| Test that an authorized user can add an entry. | |
| """ | |
| client.post('/login', data={'username': 'admin', 'password': 'default'}) | |
| response = client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'}) | |
| assert response.status_code == 302 # Redirect after successful addition | |
| def test_remove_entry_authorized_remove(self, client): | |
| """ | |
| Test that an authorized user can remove an entry. | |
| """ | |
| client.post('/login', data={'username': 'admin', 'password': 'default'}) | |
| client.post('/add', data={'title': 'Test Title', 'text': 'Test Text'}) | |
| # Get the entry ID | |
| with app.app_context(): | |
| db = get_db() | |
| entry_id = db.execute('SELECT id FROM entries WHERE title = ?', ['Test Title']).fetchone()['id'] | |
| # Remove the entry | |
| response = client.post(f'/remove/{entry_id}', follow_redirects=True) | |
| # Check that the removal was successful | |
| assert response.status_code == 200 | |
| assert b'Entry was successfully removed' in response.data | |
| # Verify the entry no longer exists | |
| with app.app_context(): | |
| db = get_db() | |
| entry = db.execute('SELECT * FROM entries WHERE id = ?', [entry_id]).fetchone() | |
| assert entry is None |
Adds delete functionality for entries in the Flask blog application. This change introduces the ability for authenticated users to remove blog entries through a delete button that appears next to each post. The implementation includes:
This enhancement improves content management capabilities while maintaining proper security controls by ensuring only authenticated users can delete entries.