for review #1
Loading…
Reference in a new issue
No description provided.
Delete branch "init"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@shuber
die letzten commits waren nur änderungen an der website
@ -0,0 +1,117 @@# Ninja-TANSS Comparison Project Ruleswieso haben wir .cursorrules? ist das von opencode?
Das habe ich hinzugefügt, habe gehofft, dass es die KI Antworten verbessert.
War aber nicht so, das entferne ich.
@ -0,0 +11,4 @@# TANSS Login Configuration (for cookie-based authentication)TANSS_LOGIN_USERNAME=your_username_hereTANSS_LOGIN_PASSWORD=your_password_heresehr ordentlich, keine passwörter gepushed, perfekt!
Danke!
@ -0,0 +1,9 @@# Secretsdie bruno-sachen könnten wir in ein separates repo auslagern, dann ist das bisschen gesammelt und wir könnten eine große collection aufbauen, die wir versionieren
Wenns für dich i.O. ist, würde ich das gegen Ende des Projekts machen. So wäre es für mich einfacher, wenn ich neue Endpunkte hinzufügen muss.
klar, passt!
@ -0,0 +6,4 @@environment:POSTGRES_USER: adminPOSTGRES_PASSWORD: secretPOSTGRES_DB: syncninjavielleicht könnten wir hier gleich noch deinen code als docker-image verschiffen. dann können wir das deployment über CI/CD automatisieren
Ja, das wäre auch mein Plan.
Wenn ich mit der Website fertig bin, würde ich das in Angriff nehmen.
passt!
@ -0,0 +1,92 @@"""CLI and API entry point for the Ninja-TANSS comparison tool."""wär cool. wenn du immer noch den author (also deinen namen dgoetz) reinschreiben würdest.
bei java gibt es da spezielle "marker" ausi (@author), vielleicht hat python sowas auch. wenn mehrere da rumpfuschen, weiß man wen man ansprechen muss bei unklarheiten
@ -0,0 +1,30 @@[project]name = "ninja-tanss-comparison"das noch anpassen bitte (und beschreibung natürlich), ansonsten tippitoppi
@ -0,0 +12,4 @@from fastapi.responses import HTMLResponse, JSONResponsefrom pydantic import BaseModel, Fieldfrom src.db.postgresql import HiddenDevice, create_tables, get_sessiondas modul postgresql könnten wir vllt umbennen. produktname gefällt mir nicht soo gut
Wie findest du:
from src.db.postgresql => from src.db.connection
@ -0,0 +77,4 @@_pipeline_state.running = False@router.post("/api/run-pipeline")versionierung würde hier noch sinn machen. also /api/v1/run-pipeline
möchte man etwas ändern aber was altes nicht zerschießen, kann man über api-versionierung machen
EDIT: vllt kann man auch nur .../pipeline/ schreiben statt run-pipeline, das ist dann bisschen cleaner
Also dann /api/v1/pipeline ?
@ -0,0 +94,4 @@return JSONResponse(status_code=200, content={"status": "started"})@router.websocket("/api/pipeline-ws")man könnte hier vielleicht das ws vorne in den pfad einbauen falls man mehrere ws-anwendungsfälle hat, also /api/v1/ws/pipeline oder so
@ -0,0 +114,4 @@passclass EntityEntry(BaseModel):das gehört sich glaub ich nicht in die routes.py, oder?
@ -0,0 +125,4 @@tanss_contract: TanssContract | None = Noneclass InvalidStateEntry(BaseModel):gleiches hier
@ -0,0 +133,4 @@entities: list[EntityEntry] = Field(..., description="List of related entities")class InvalidStatesResponse(BaseModel):gleiches hier
@ -0,0 +150,4 @@)@router.get(wollen wir das vielleicht zu /states/invalid oder so ändern? dann würden wir diesen bindestrich wegbekommen
@ -0,0 +165,4 @@return store.invalid_statesclass SystemMetadata(BaseModel):das müsste alle wsl ins db-modul, sind ja datenbanktabellen
@ -0,0 +61,4 @@return []def saveCache(filename: str, data: list) -> None:das ist obsolet, wir haben inzwischen ja eine datenbakn, doer?
@ -0,0 +31,4 @@Base.metadata.create_all(bind=engine)def get_session():könnten wir zu singleton umbauen, dann kannst du jedes mal getSession() aufrufen und musst dir das Objekt nicht speichern. Dann haben wir eine Session über den Scope der ganzen Applikaiton
@ -0,0 +5,4 @@from .logger import loggerdef printFetching(message: str) -> None:das versteh ich nicht, wieso rufen wir nicht direkt den logger auf?
Danke, das ist ein Restbestand, der kommt weg. Ist mir nicht aufgefallen.
@ -0,0 +14,4 @@logger.info(message)def getNested(data: dict, dot_path: str, default=None):vielleicht kann man sich mal überlegen, ob man sowas auslagert in ein tanss-modul. vllt könnten wir uns da auch mal was mit ki basteln, um die tanss-api mit python zu bedienen, so eine standard-library. das könnte für odoo auch gut werden.
aber für jetzt: vielleicht in ein tanss/utilities modul oder so, dann kann man das wiederverwenden für zukünftige projekte
Ich habe mich dagegen entschieden, das in den tanss ordner zu verschrieben, weil es eine sehr generische funktion ist und nicht viel mit tanss zu tun hat
@ -0,0 +4,4 @@import syslogging.basicConfig(level=logging.INFO,loglevel sollte setzbar über umgebungsvariable sein
@ -0,0 +9,4 @@handlers=[logging.StreamHandler(sys.stdout)],)logger = logging.getLogger("ninja-tanss")den logger-namen könnten wir vllt umbennen zum app-namen
@ -0,0 +28,4 @@Priority.CRITICAL: logging.ERROR,Priority.HIGH: logging.WARNING,Priority.MEDIUM: logging.INFO,Priority.LOW: logging.DEBUG,bitte kein debug für logs für diese flughöhe der logik verwenden. debug soll ja eher helfen das programm zu verstehen, das soll ja hinweisen, dass was nicht hinhaut mit dem datenbestand
@ -0,0 +22,4 @@from ..logger import loggerfrom .oauth2 import oauth# Hardcoded endpointsdie base_url sollten wir auslagern. man weiß ja nie
@ -0,0 +137,4 @@url = f"{NINJA_API_BASE_URL}{NINJA_CUSTOM_FIELDS_ENDPOINT.format(id=device_id)}"for attempt in range(max_retries):diese retry-logik würde ich auch in einer anderen methode sehen. dann kann man das recyclen
Die wird aktuell nur in der Datei benötigt. Ich würde das erst auslagern, wenn es noch einen Use-Case dafür gibt.
iO
@ -0,0 +212,4 @@device.tanssId = fields[0]device.teamviewerId = fields[1]saveCache("ninja_devices.json", [d.__dict__ for d in result])wie besprochen
@ -0,0 +32,4 @@return match.group(1) if match else Noneclass OrganizationSchema(Schema):auch hier könnten wir das vielleicht bisschen ausbauen und in eine ninja library packen. das wäre natürlich cool, weil wir das für odoo auch recyclen könnten.
auf jeden fall auslagern, wenn möglich. weißt du wie ich mein?
Ich weiß gerade nicht, wie ich das umsetzen soll. Das würde bedeuten, dass man zwei neue Repos für tanss und ninja anlegt und diese dann irgendwie über uv runterlädt und dann importiert?
später einmal, und du könntest den grundstein dafür legen, genau
einfach alles ninja-ige möglichst generisch schreiben und in ein modul, das man später rausnehmen kann
@ -0,0 +13,4 @@## Hardcoded endpointNINJA_API_BASE_URL = "https://datajob.rmmservice.eu"das könnte man zu einer variable machen
@ -0,0 +47,4 @@tanss_display_id = ninja_org.get_tanss_display_id()if tanss_display_id is None:if ninja_org.name == "Meißner EDV - Kunden":könnten wir den namen in eine variable speichern und nicht hardcoden?
Ich weiß nicht, ob das den Aufwand und die Logik wert ist.
Das hier soltle ein Einzelfall sein und auf absehbare zeit auch bleiben
ist tatsächlich aber trotzdem eleganter, dann kann man nämlich auch die variable benennen und einen schönen kommentar hinzufügen. so denk ich mir: was geht denn hier ab
@ -0,0 +50,4 @@if ninja_org.name == "Meißner EDV - Kunden":# => that's okcontinueelif ninja_org.name == "Neue Geräte":s. oben
@ -0,0 +17,4 @@logger = logging.getLogger(__name__)def phase2(ich hab ja schon gesagt, dass ich mit den namen nicht so zufrieden bin. vielleicht könntest du da was anderes finden als phaseX, das ein bisschen ausdrucksstärker ist
@ -0,0 +142,4 @@days_since_contact = (datetime.now().date() - last_contact_date).daysif days_since_contact > 90: # 3 monthslogger.debug(hier debug vllt wie gesagt abändern
und die logs kommen ja nie durch, loglevel ist ja auf "info" gesetzt
Mittlerweile kann man das log level in der .env einstellen. Soll ich es dennoch ändern?
ja bitte
@ -0,0 +166,4 @@)continueelse:if tanss_device.is_peripherie:englischer begrifff bitte (periphery)
@ -0,0 +42,4 @@passelif tanss_device.teamviewerId != "" and ninja_device.teamviewerId is not None:if tanss_device.teamviewerId != ninja_device.teamviewerId:logger.debug(f"TeamviewerId mismatch for {tanss_device=} and {ninja_device=}")natürlich hier dann auch überall logger.debug ersetzen
@ -0,0 +14,4 @@from .devices import getTanssDevicesUSERNAME = "user"PASSWORD = "HIwURXsB02OECXkk37W5"🫣
Upsi
@ -0,0 +55,4 @@def getTanssCompany(company_id: str) -> TanssCompany:"""Fetch a single TANSS company by company_id."""response = get(f"https://nodered.dev.datajob.de/tanss-wrapper/v1/companies/{company_id}",hier kann man die url auch konfigurierbar machen
@ -0,0 +159,4 @@next_batch: list[int] = list(tanss_company_ids)# Fetch companies and their linked companies in batches using threadswith tqdm(unit="company") as pbar:da wären aussagekräftigere namen besser.
gut ist, wenn der code innerhalb 10 sekunden klar wird und man den kontext aus den namen ableiten kann - das hat hier leider nicht geklappt
EDIT: ich sehe, dass tqdm der name einer library. dann vllt efinach progress_bar statt pbar und einen kommentar
@ -0,0 +98,4 @@A list of TanssContract objects containing id, from_date, to_date, and contract_name."""response = get(f"https://ticket.datajob.de/ajax/srv/gv_v4.php?t=showWvTasksInfo&type=1&id={device_id}",URL umgebungsvariable
@ -0,0 +157,4 @@devices = [d for d in getTanssDevices() if not d.is_peripherie]all_contracts: list[TanssContract] = []printFetching(f"Fetching TANSS contracts: {len(devices)} items with {MAX_WORKERS} workers")das muss man vereinheitlichen, auch über logging.info
Was meinst du genau? Mittlerweile wurde printFetching durch logger.info ersetzt
genau das meint ich!
@ -0,0 +204,4 @@Returns:The matching Contract enum value, or UNKNOWN if no match."""if (das lieber als variablen speichern bitte
@ -0,0 +10,4 @@from ..helper import printFetching# Hardcoded base URLTANSS_API_BASE_URL = "https://nodered.dev.datajob.de"env-var hier wieder
EDIT: ah und der name ist irreführend: das ist ja nicht die TANSS_API, das ist der node-red wrapper
@ -0,0 +1,651 @@<!DOCTYPE html>kannst du der ki vllt sagen, sie soll separate css und js-dateien nehmen? so ist das ja grausam
Die index.html ist eigentlich auch nicht zum Verstehen da
Aber ich geb's weiter :D
@dgoetz ist fertig
@opencode siehst du noch etwas?