Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin customization #129

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

jupadin
Copy link
Contributor

@jupadin jupadin commented Oct 23, 2023

Change Types

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Anpassung des Admin-Portals nahezu aller (außer Player-App) Apps bzw. Modellen.

  • Konkretisierung von Modell-Imports.
  • Benennung bzw. Hinzufügen von Variablenklarnamen (Internalization als Feature-Request bzw. To-Do in separatem PR).
  • Styling einiger Admin-Seiten zur einfacheren Übersicht

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read CONTRIBUTING.md.
  • I have added tests to cover my changes.
  • My tests run automatically.
  • All new and existing tests passed.

@jupadin jupadin requested a review from djbrown as a code owner October 23, 2023 18:10
@djbrown
Copy link
Owner

djbrown commented Oct 23, 2023

hey @jupadin :) habe eben diesen Pull Request gesehen.
danke schon mal dafür 😉

Die Konkretisierung der Imports finde ich gut, die relativen Imports waren mir bisher gar nicht aufgefallen.
Die Umstrukturierung der Admin-UI muss ich mir nochmal im laufenden System anschauen, vermute das kann auch gemergt werden.

Bezüglich der Variablenklarnamen würde ich gerne den Hintergrund verstehen. Nutzt du die Software selber und hast die Änderungen für dich als Admin gemacht, oder quasi für mich als Admin der zentralen Instanz (hbscorez.de), oder hat der PR einen anderen Hintergrund? Je nachdem was die Intention ist, kann ich besser entscheiden. Internationalisierung habe ich nämlich definitv noch auf der Liste der Features, allerdings eher für Benutzer. Mittelfristig möchte ich zunächst die Daten der anderen deutschen Handballplattform SIS-Handball (und oder Handball.net) anbinden, und langfristig auch andere Länder. Ich hätte gerne daher die Namen im Backend noch auf englisch. Außerdem ist für mich so der Bezug zum Code besser erkenntlich.

Abgesehen vom Codereview bitte noch den mypy-Fehler beheben:
src/associations/management/commands/import_associations.py:61: error: Unexpected attribute "source_url" for model "Association" [misc]
Das Attribut scheint beim Bearbeiten entfernt worden zu sein. Ich hatte es explizit aufgenommen, da die neuen Verbands-URLs auf der Portalstartseite sich nicht aus anderen Attributen ableiten lassen.

Und bitte noch den Feature-Branch vom Hauptbranch (djbrown/hbscorez#master) updaten. ich hoffe meine Änderungen in den letzten Tagen (Restrukturierung der Commands) kommen beim mergen nicht zu sehr in die Quere 😬 hatte nicht mitbekommen, dass du am Projekt arbeitest, sonst hätte ich es vermutlich inhaltlich und zeitlich anders gemacht

Copy link
Owner

@djbrown djbrown left a comment

Choose a reason for hiding this comment

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

s. auch letzter Kommentar

Comment on lines +16 to +19
migrations.RemoveField(
model_name="association",
name="source_url",
),
Copy link
Owner

Choose a reason for hiding this comment

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

Attribut source_url wird nach meinem Verständnis noch benötigt

name = models.TextField(unique=True)
abbreviation = models.TextField()
bhv_id = models.IntegerField(unique=True)
source_url = models.TextField()
Copy link
Owner

Choose a reason for hiding this comment

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

Attribut source_url wird nach meinem Verständnis noch benötigt

@jupadin
Copy link
Contributor Author

jupadin commented Oct 24, 2023

Hi @djbrown,

das ging fix. Danke für dein Feedback.
Der PR dient hauptsächlich dir als Admin der Hauptinstanz (hbscorez.de) bzw. möglichen weiteren Instanz-Maintainern 😏.

Die Variablenklarnamen kamen mehr aus Angewohnheit diese gleich mitzuführen bzw. für die Internationalisierung.
Klar, es macht definitiv mehr Sinn diese erstmal für die Website bzw. Front-End einzuführen - hierfür werden diese nicht benötigt. Für die Darstellung im Admin-Bereich sind sie allerdings wieder hilfreicher, da dann nicht für jede Spalte bzw. display-name eine separate Funktion geschrieben werden muss.
Falls gewünscht, kann ich sie natürlich auch wieder entfernen - Update des PR kommt :)

Analog dazu auch das Update vom Feature-Branch auf die Änderungen des Hauptbranches (djbrown/hbscorez#master). Damit sollte sich auch der mypy-Fehler erledigen bzw. das Rollback bezüglich der source_url-Variablen nötig sein. Deine aktuelle Änderung hatte ich hierzu nicht im Branch.

Lass uns gerne nochmal enger in die Abstimmung gehen :)
Neben der Internationalisierung und Integration weiterer Plattformen (SIS-Handball, etc.) finde ich u.a. die 7m-Statistik interessant (https://redmine.djbrown.de/issues/20) bzw. weitere Spiel-Statistiken spannend.

Viele Grüße

djbrown added a commit that referenced this pull request Oct 30, 2023
@jupadin
Copy link
Contributor Author

jupadin commented Oct 31, 2023

Hi @djbrown,

ich habe mir die aktuellen Änderungen gerade nochmal angesehen und meine, dass sich die URL aus den bestehenden Informationen (abbreviation und NEW_ROOT_SOURCE_URL) zusammenbauen lässt.

Dies erfolgt über 'https://www.handball4all.de/' + 'home/portal/' + association.abbreviation.lower()

Am Beispiel des SHV:

url = f'{settings.NEW_ROOT_SOURCE_URL}/home/portal/{association.abbreviation.lower()}'
wird zu
https://www.handball4all.de/home/portal/shv
bzw. im Browser ein Redirect zu
https://www.handball4all.de/home/portal/shv#/schedule

Die Überprüfung, ob ein Verband bereits besteht und ggfs. dessen Name aktualisiert werden muss, habe ich im aktuellen Commit angepasst, sodass die Variable source_url nicht weiter benötigt wird.

Gerne Feedback hierzu und viele Grüße

@djbrown
Copy link
Owner

djbrown commented Oct 31, 2023

@jupadin die "Kürzel" aus dem H4A-Portal entsprechen teilweise nicht den "offiziellen" Kürzeln der Verbände.
Aktuell sind auf hbscorez.de noch die gleichen "Kürzel" wie im Portal hinterlegt.
Durch die Trennung zwischen Portal-URL und Kürzel kann ich das fixen, habe eben schon mal die Pfalz umgestellt (PFALZ->PfHV)
grafik
grafik

@jupadin
Copy link
Contributor Author

jupadin commented Nov 2, 2023

Ja, die offiziellen "Kürzel" und die URL stimmen nicht zwangsweise überein (s. dein Beispiel "PfHV").
Die offiziellen "Kürzel" können jedoch über die URL, welche auf der H4A-Homepage (mit dem inoffiziellen "Kürzel"), gefunden werden.
Über die URL https://www.handball4all.de/home/portal/pfalz/#/schedule kommen wir auf die Seite des Pfälzer Handball-Verbands. Von dort erfolgt ein Request an die "JSON-API" bzw. ein PHP-Skript mit https://spo.handball4all.de/service/if_g_json.php?cmd=po&og=171, wobei 171 die bhv_id des PfHV ist. Über die Response kann über das Feld sname das "Kürzel" gefunden werden.

Somit kann die bisherige Struktur erhalten bleiben und trotzdem das offizielle "Kürzel" verwendet werden. Hier ist auch die reportURL und weitere Infos, wie bspw. die letzte Aktualisierung zu finden.

Copy link

sonarcloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants