2
\$\begingroup\$

I've created a Flask application that processes Excel files uploaded by users. The application reads specific data based on client-specific keywords and generates a CSV file which users can download. I'm looking for feedback on improving the structure, efficiency, and security of this application.

The whole app idea was to normalize the XLSX/XLS files and extract the most important data. The program is not perfect, I want to make it more dynamic, when it comes to sheet-name or other mechanisms inside.

This is only backend code.

The goal was to create the standardized version of XLSX file, so the user will be able to import order into the Prodio.

The important thing that I couldn't achieve is that, when our header columns has different name, for example Reference should also be Referencja etc.... I was getting a lot of errors, but the rest isn't that bad... is it?

from flask import Flask, request, send_file, render_template, redirect, url_for, session, make_response, flash
import re
from werkzeug.utils import secure_filename
import os
import pandas as pd
from io import StringIO
import logging
from dataclasses import dataclass, field
from typing import Optional, Dict
from datetime import datetime
from gevent.pywsgi import WSGIServer

app = Flask(__name__)
app.config['UPLOAD_FOLDER'] = 'uploads/'
app.config['ALLOWED_EXTENSIONS'] = {'xlsx', 'xls'}
app.secret_key = 'super secret key'

os.makedirs(app.config['UPLOAD_FOLDER'], exist_ok=True)

logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(levelname)s - %(message)s')

def load_product_mapping():
    product_mapping_df = pd.read_csv('products_data.csv')
    product_mapping_df['Dodatkowe oznaczenia'] = product_mapping_df['Dodatkowe oznaczenia'].fillna('').astype(str).str.strip()
    product_mapping_df['Nazwa'] = product_mapping_df['Nazwa'].str.strip()
    product_mapping = {k: v for k, v in product_mapping_df.set_index('Nazwa')['Dodatkowe oznaczenia'].items() if v}
    return product_mapping

@dataclass
class ExcelProcessorConfig:
    client_keyword: str = 'Valeo Electric'
    date_keywords: Dict[str, str] = field(default_factory=lambda: {
        'pick_up': 'Pick-up',
        'receiving': 'Receiving'
    })
    columns: list = field(default_factory=lambda: [
        'Planner', 'Reference', 'Description', 'Location', 'Unit', 'Details'
    ])

    column_mapping: Dict[str, str] = field(default_factory=lambda: {
        'Reference': ['Reference', 'REFERENCJA'],
        'Description': ['Description', 'OPIS'],
    })
    
    exclusion_keyword: str = 'Transportation mode'
    final_columns: list = field(default_factory=lambda: [
        'Klient', 'Oczekiwany termin realizacji', 'Termin potwierdzony', 'Reference', 'Produkt', 'Sztuk',
        'Uwagi dla wszystkich', 'Uwagi niewidoczne dla produkcji', 'Atrybut 1 (opcjonalnie)', 
        'Atrybut 2 (opcjonalnie)', 'Atrybut 3 (opcjonalnie)'
    ])

class ExcelProcessor:
    def __init__(self, config: ExcelProcessorConfig):
        self.config = config

    def extract_client_name(self, data: pd.DataFrame) -> Optional[str]:
        try:
            row = data[data.iloc[:, 3].str.contains(self.config.client_keyword, na=False)].iloc[0, 3]
            return row.split('\n')[0].split(',')[0]
        except IndexError as e:
            logging.error(f"Failed to extract client name: {e}")
            return None
        
    def find_relevant_sheet(self, excel_data: pd.ExcelFile) -> Optional[pd.DataFrame]:
        priority_sheets = ['Antalis', 'Pick-up order ATK', 'ATK', 'Sheet1']
        for sheet_name in priority_sheets:
            if sheet_name in excel_data.sheet_names:
                return pd.read_excel(excel_data, sheet_name=sheet_name)

        for sheet_name in excel_data.sheet_names:
            df = pd.read_excel(excel_data, sheet_name=sheet_name)
            if self.config.client_keyword in df.to_string():
                return df
        return None

    def extract_date(self, data: pd.DataFrame, keyword: str) -> str:
        try:
            logging.debug(f"Searching in DataFrame column: {data.iloc[:, 0].tolist()}")

            pattern = re.compile(r'\b' + re.escape(keyword) + r'\b', re.IGNORECASE)
            
            for index, row in data.iterrows():
                for col_index, cell_value in enumerate(row):
                    if pattern.search(str(cell_value)):
                        desired_data = None
                        for col in range(col_index + 1, len(row)):
                            if pd.notna(data.iloc[index, col]):
                                desired_data = str(data.iloc[index, col])
                                break
                        if desired_data is not None:
                            return desired_data
                        else:
                            return "Unknown date"
            
            logging.warning(f"No matches found for keyword '{keyword}'. Data might not contain the expected keywords.")
            return "Unknown date"
        
        except Exception as e:
            logging.error(f"An error occurred while extracting the date: {e}")
            return "Unknown date"

    def prepare_order_data(self, data: pd.DataFrame) -> pd.DataFrame:
        try:
            start_row = data[data.iloc[:, 0].str.contains('DB', na=False)].index[0]
            order_data = data.iloc[start_row:, :6]
            order_data.columns = self.config.columns
            return order_data.dropna(subset=['Reference']).query(f"Reference != '{self.config.exclusion_keyword}'")
        except IndexError as e:
            logging.error(f"Error processing order data: {e}")
            return pd.DataFrame()
        
    def process_file(self, file_path: str) -> pd.DataFrame:
        try:
            excel_data = pd.ExcelFile(file_path)
            antalis_data = self.find_relevant_sheet(excel_data)

            client_name = 'Valeo'
            pick_up_date = '19.08.2024'
            receiving_date = self.extract_date(antalis_data, self.config.date_keywords['receiving'])

            order_data = self.prepare_order_data(antalis_data)
            if not order_data.empty:
                order_data['Klient'] = client_name
                order_data['Oczekiwany termin realizacji'] = pick_up_date
                order_data['Termin potwierdzony'] = ""
                order_data['Produkt'] = order_data['Description'] + ' ' + order_data['Location'].fillna('')

                # product_mapping = load_product_mapping()
                # product_mapping = {k.strip(): v.strip() for k, v in product_mapping.items()}
                # order_data['Produkt'] = order_data['Produkt'].str.strip()
                # print("Data Before Mapping:", order_data['Produkt'].tolist())
                # order_data['Produkt'] = order_data['Produkt'].map(product_mapping).fillna(order_data['Produkt'])
                # print("Data After Mapping:", order_data['Produkt'].tolist())
                order_data['Sztuk'] = order_data.apply(lambda row: row['Details'] if pd.notna(row['Details']) and row['Details'] != 'NA' else '', axis=1)
                order_data = order_data[order_data['Sztuk'] != '']

                for column in self.config.final_columns:
                    if column not in order_data.columns:
                        order_data[column] = ''
                final_data = order_data[self.config.final_columns]
                final_data.columns = ['Klient', 'Oczekiwany termin realizacji', 'Termin potwierdzony', 'Zewn. nr zamówienia', 'Produkt', 'Sztuk', 'Uwagi dla wszystkich', 'Uwagi niewidoczne dla produkcji', 'Atrybut 1 (opcjonalnie)', 'Atrybut 2 (opcjonalnie)', 'Atrybut 3 (opcjonalnie)']

                return final_data
            else:
                return pd.DataFrame()
        except Exception as e:
            logging.error(f"An error occurred while processing the Excel file: {e}")
        return pd.DataFrame()

@app.route('/', methods=['GET', 'POST'])
def upload_file():
    if request.method == 'POST':
        if 'file' not in request.files:
            flash('Brak pliku!', 'error')
            return redirect(url_for('upload_file'))
        file = request.files['file']
        if file.filename == '':
            flash('Nie wybrałeś pliku!', 'error')
            return redirect(url_for('upload_file'))
        if file and allowed_file(file.filename):
            filename = secure_filename(file.filename)
            file_path = os.path.join(app.config['UPLOAD_FOLDER'], filename)
            file.save(file_path)

            processed_data = processor.process_file(file_path)
            if not processed_data.empty:
                session['data'] = processed_data.to_csv(index=False, sep=';')
                return render_template('upload.html', data=processed_data.to_html(classes='data', index=False), has_data=True)
            else:
                flash('Brak informacji, bądź nie można ich znaleźć.', 'error')
                return redirect(url_for('upload_file'))
        else:
            flash('Nieprawidłowy typ pliku', 'error')
            return redirect(url_for('upload_file'))
    else:
        return render_template('upload.html', data=None, has_data=False)

@app.route('/normalizer', methods=['GET'])
def display_vr():
    if request.method == 'GET':
        return render_template('normalizer.html');  

@app.route('/pobierz', methods=['GET'])
def download_file():
    if 'data' in session:
        csv_data = pd.read_csv(StringIO(session['data']), sep=';')
        client_name = csv_data['Klient'].iloc[0] if not csv_data['Klient'].empty else 'default'

        today = datetime.now().strftime('%Y-%m-%d')

        filename = f"{client_name}_{today}.csv"

        response = make_response(session['data'])
        response.headers["Content-Disposition"] = f"attachment; filename={filename}"
        response.headers["Content-Type"] = "text/csv"
        return response
    else:
        return redirect(url_for('upload_file'))

@app.route('/edycja', methods=['POST'])
def adjust_data():
    if 'data' in session:
        client_name = request.form.get('client_name', None)
        expected_date = request.form.get('expected_date', None)
        confirmed_date = request.form.get('confirmed_date', None)

        csv_data = pd.read_csv(StringIO(session['data']), sep=';')

        if client_name is not None and client_name.strip():
            csv_data['Klient'] = client_name.strip()

        if expected_date is not None and expected_date.strip():
            expected_date = datetime.strptime(expected_date.strip(), '%d.%m.%Y').strftime('%Y-%m-%d')
            csv_data['Oczekiwany termin realizacji'] = expected_date

        if confirmed_date is not None and confirmed_date.strip():
            confirmed_date = datetime.strptime(confirmed_date.strip(), '%d.%m.%Y').strftime('%Y-%m-%d')
            csv_data['Termin potwierdzony'] = confirmed_date

        updated_csv = csv_data.to_csv(index=False, sep=';', na_rep='')
        session['data'] = updated_csv

        return render_template('upload.html', data=csv_data.to_html(classes='data', index=False, na_rep=''), has_data=True)
    else:
        return redirect(url_for('upload_file'))

@app.route('/adjust_client_name', methods=['POST'])
def adjust_client_name():
    if 'data' in session:
        client_name = request.form.get('client_name', None)
        
        csv_data = pd.read_csv(StringIO(session['data']))

        if client_name is not None and client_name.strip():
            csv_data['Klient'] = client_name.strip()

        updated_csv = csv_data.to_csv(index=False, na_rep='')
        session['data'] = updated_csv

        return render_template('upload.html', data=csv_data.to_html(classes='data', index=False, na_rep=''), has_data=True)
    else:
        return redirect(url_for('upload_file'))

def allowed_file(filename):
    return '.' in filename and filename.rsplit('.', 1)[1].lower() in app.config['ALLOWED_EXTENSIONS']

if __name__ == '__main__':
    config = ExcelProcessorConfig()
    processor = ExcelProcessor(config)
    # http_server = WSGIServer(('', 3000), app)
    # http_server.serve_forever()
    app.run(host="0.0.0.0", port=3000)

Example data:

https://docs.google.com/spreadsheets/d/1tJY9LebEV8q6Yn_sbm0_Ocl6xNTxJ2EPCLx-aP73zqM/edit?usp=sharing

How can I improve the separation of concerns, especially separating the web handling logic from the data processing?

Are there any obvious security flaws in the way I handle file uploads and session management?

How can I make the error handling more robust and less invasive?

What improvements can be made to the configuration management to make the app more maintainable?

\$\endgroup\$
0

1 Answer 1

4
\$\begingroup\$

Disclaimer: I haven't run your code, so this is a static analysis exercise based on what I have noticed while perusing it.

Routes

This method only accepts GET requests:

@app.route('/normalizer', methods=['GET'])
def display_vr():
    if request.method == 'GET':
        return render_template('normalizer.html');  

Therefore, the if request.method == 'GET': statement is redundant.

Likewise, you could streamline your routes a bit. Instead of having a route that accepts both GET and POST:

@app.route('/', methods=['GET', 'POST'])
def upload_file():

Just split the code in two routes:

@app.route('/', methods=['POST'])
def upload_file():

@app.route('/', methods=['GET'])
def show_upload_file():

Then you can remove a few conditional statements, like if request.method == 'POST':. It is perfectly fine to handle the requested route on a per-method basis, but the Python function names have to be unique of course. Not many tutorials show that pattern, so many Flask developers are not aware probably.

File validation

Function allowed_file could be replaced with pathlib: PurePath(filename).suffix. Pathlib can also replace os.path.join, save etc

It is good that you are using secure_filename to sanitize the file name. But you could still overwrite an existing file. Maybe this is not a concern to you.

Logging

Good points: usage of logging, including for debugging.

Comments and naming

A few docstrings/comments would be welcome. For instance, looking at functions like prepare_order_data, the function name is so broad that it does not tell us what it does exactly. The only way is to parse the code to figure it out. find_relevant_sheet is mysterious as well, because the definition of "relevant" in this context is not stated explicitly.

On the other hand, extract_date is quite telling, and we even have the type hint which is nice. Ditto for extract_client_name.

Looping

Some loops could be made more succinct, consider this:

            if pattern.search(str(cell_value)):
                desired_data = None
                for col in range(col_index + 1, len(row)):
                    if pd.notna(data.iloc[index, col]):
                        desired_data = str(data.iloc[index, col])
                        break
                if desired_data is not None:
                    return desired_data
                else:
                    return "Unknown date"
              

You can return early instead of breaking out of the loop. break is used when you intend to continue running some code past the loop. But there is no more code in this function. The additional if statements are unnecessary and just bring bloat and distraction. Just take care of indentation:

            if pattern.search(str(cell_value)):
                for col in range(col_index + 1, len(row)):
                    if pd.notna(data.iloc[index, col]):
                        return str(data.iloc[index, col])

                return "Unknown date"

In fact, you are already using that pattern in find_relevant_sheet. You know this already.

This is still not optimal. Since you are looking up for a specific value/pattern in a dataframe, there are filtering expressions you can use in Pandas. You normally wouldn't do a loop for this. See for example: 8 Ways to Filter Pandas DataFrames

More Pandas stuff

I am not a Pandas expert, but I can see a few areas where it is underutilized. For example, in load_product_mapping you do some trimming of whitespace, which should be fast enough in its own right because Pandas is fast and you don't seem to be handling large files.

def load_product_mapping():
    product_mapping_df = pd.read_csv('products_data.csv')
    product_mapping_df['Dodatkowe oznaczenia'] = product_mapping_df['Dodatkowe oznaczenia'].fillna('').astype(str).str.strip()
    product_mapping_df['Nazwa'] = product_mapping_df['Nazwa'].str.strip()
    product_mapping = {k: v for k, v in product_mapping_df.set_index('Nazwa')['Dodatkowe oznaczenia'].items() if v}
    return product_mapping

pandas.read_csv has many arguments, but you are not using any (beyond the separator), yet I am sure that some would be useful, like skipinitialspace. I would try this rather than apply strip() etc. The idea is to acquire the file at the source in the most optimal way possible, rather than do cleanup afterwards.

Class

+1 for using @dataclass. Since you are already familiar with, other classes like ExcelProcessor could be upgraded as well.

Structure

Here it is normal to post MRE (Minimal reproducible example) code. In production, you would split the code in a few files, that you import as required. For example, the ExcelProcessor class can and should go to a separate file. It's easier to maintain code that is short and well delimited.

In fact, the routes can have their own file too, especially when you use Flask blueprints. When the app is small like in this case, you don't bother. If you have a lot of routes, then it can make sense to externalize that part.

On the other hand, one thing that could be detached from the main code is the configuration options like secret key, file paths etc. The idea is to be able to make changes easily, or have different settings for dev and prod, dev machine and server, without touching the codebase. Also, if you use versioning software like git, you don't want to commit secrets, especially when the code is pushed to a public repo like Github. Environment variables (using .env files) can be used for storing sensitive values.

Recommended reading: Configuring Your Flask App

Session

Instead of using session to pass data like session['data'], you could just pass a reference to a file name. The uploaded file should be stored in the temp directory.

Misc

You have multiple bits of code like this:

if client_name is not None and client_name.strip():
    csv_data['Klient'] = client_name.strip()

client_name.strip() need not be in the if statement. Just strip it.


has_data is probably redundant:

return render_template('upload.html', data=None, has_data=False)

return render_template('upload.html', data=processed_data.to_html(classes='data', index=False), has_data=True)

Your Jinja template can evaluate the data being passed is None or zero length.

Validation

In adjust_data, you are processing form data:

expected_date = request.form.get('expected_date', None)
confirmed_date = request.form.get('confirmed_date', None)

But further down, you apply strptime on the dates. This code could crash if the input format is not correct. Your Flask page should make it clear what format is expected, unless you use a datepicker in your UI. But you still have to check that incoming data. A small dedicated function could be written to validates dates, not just the format but the date value. For example, 30 February should be rejected.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you @Kate, good review! "I haven't run your code" That's normal. The responsibility of Author is to convince Reviewer not merely that the code is Correct, but that it is Obviously Correct. Feel free to do more with the code beyond reading it, but for many reviews just that much should suffice. // Suppose we're interested in e.g. performance. Then a ReadMe or similar document should summarize the Author's measurements and observations, and be part of the review submission. Why? So there's a record in git a year from now when some maintainer revisits "why is this slow?". \$\endgroup\$
    – J_H
    Commented Apr 24 at 21:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.