Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 12 additions & 1 deletion flaskr/flaskr.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def close_db(error):
@app.route('/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Suggested change
@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)

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')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Suggested change
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'])

entries = cur.fetchall()
return render_template('show_entries.html', entries=entries)

Expand Down Expand Up @@ -96,3 +96,14 @@ def logout():
session.pop('logged_in', None)
flash('You were logged out')
return redirect(url_for('show_entries'))


@app.route('/delete/<int:entry_id>', methods=['POST'])
def delete_entry(entry_id):
if not session.get('logged_in'):
abort(401)
db = get_db()
db.execute('DELETE FROM entries WHERE id = ?', [entry_id])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Suggested change
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'))

db.commit()
flash('Entry was successfully deleted')
return redirect(url_for('show_entries'))
14 changes: 14 additions & 0 deletions flaskr/static/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,20 @@ input[type="submit"]:hover {
background: var(--secondary-color);
}

.delete-entry {
margin-top: 0.5em;
}

.delete-entry input[type="submit"] {
background: var(--accent-color);
padding: 0.4em 0.8em;
font-size: 0.9em;
}

.delete-entry input[type="submit"]:hover {
background: #c0392b;
}

/* Responsive adjustments */
@media (max-width: 840px) {
.page {
Expand Down
9 changes: 8 additions & 1 deletion flaskr/templates/show_entries.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,14 @@
{% endif %}
<ul class=entries>
{% for entry in entries %}
<li><h2>{{ entry.title }}</h2>{{ entry.text|safe }}
<li>
<h2>{{ entry.title }}</h2>{{ entry.text|safe }}
{% if session.logged_in %}
<form action="{{ url_for('delete_entry', entry_id=entry.id) }}" method=post class=delete-entry>
<input type=submit value="Delete" onclick="return confirm('Are you sure you want to delete this entry?');">
</form>
{% endif %}
</li>
{% else %}
<li><em>Unbelievable. No entries here so far</em>
{% endfor %}
Expand Down
44 changes: 44 additions & 0 deletions tests/test_flaskr.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,51 @@ def test_show_entries(self):
# 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(self):
"""
Test that an unauthorized user cannot delete entries.
"""
with app.test_client() as client:
# 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(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not able to suggest a fix for this review finding.

Request ID : f95047e6-ed2b-45db-8527-d6581605bb4e

"""
Test that an authorized user can delete entries.
"""
with app.test_client() as client:
# First, log in
client.post('/login', data={
'username': app.config['USERNAME'],
'password': app.config['PASSWORD']
})

# Add an entry to delete
client.post('/add', data={
'title': 'Test Entry to Delete',
'text': 'This entry will be deleted'
})

# Get the entries to find the ID of the one we just added
with app.app_context():
db = get_db()
entry = db.execute('SELECT id FROM entries WHERE title = ?',
['Test Entry to Delete']).fetchone()

# Now delete the entry
response = client.post(f'/delete/{entry["id"]}', follow_redirects=True)

# Check if deletion was successful
assert response.status_code == 200
assert b'Entry was successfully deleted' in response.data

# Verify the entry is no longer in the database
deleted_entry = db.execute('SELECT * FROM entries WHERE id = ?',
[entry["id"]]).fetchone()
assert deleted_entry is None

class AuthActions(object):

Expand Down