Geocaches approval refactoring, fixes #1672, affects #48

[rapotek]

The main cause to do geacaches approval refactoring was unification of sending emails using Email object everywhere, but the code was so deprecated, the full refactoring was needed.

Main changes:

• The whole functionality is moved from a static HTML page to a dynamic (jQuery + jQuery UI + API) one
• The list of caches waiting for acceptance is retrieved in one big query containing all necessary data instead of a bunch of queries per cache
• The emails are formatted using subject, headers and footers commonly used in another email messages
• Additional information is supplied regarding accepted/rejected caches, it can help an OC team member to remind last action when distracted

One additional external JS libraries are used:

• moment.js, useful for date and time manipulation, included as a chunk with remote source

The client side has been tested successfully on: Mozilla Firefox 100, Google Chrome 101, MS Edge 101 and Opera 86.

Some visual examples:

• The list of caches waiting for acceptance: Before: After:
• Cache assigned to reviewer: Before: After:
• Cache accept confirmation: Before: After:
• Cache accepted: Before: After:
• No caches to accept: Before: After:
• Email sent to a cache owner: Before: After:

[deg-pl]

I think we can merge this PR. I have only one comment - see above. @kojoty what do you think about it? @rapotek thanks for your contribution. This is the next step to better OC PL code

[kojoty]

Hi @rapotek

I still don't have much time for OC and I would like not to block anything here.

Anyway: handlebars vs jsRender is a thing which I would like to discuss.

At first I have nothing to jsRender - I have never used it, maybe it's OK but I think we should not use next new template system If the issue is {{}} in context of translations I think we should not use it at all in php code and use <?=tr('xyz')?> instead. The reason is that if I remember well it needs to parse all the code with eval in php so I think this is not the best way - I don't have a time to check it now but I remember I disliked {{xyz}} translations because of something and wanted to remove them at all.

The rest seems to be OK.

[rapotek]

Hi @rapotek

I still don't have much time for OC and I would like not to block anything here.

Anyway: handlebars vs jsRender is a thing which I would like to discuss.

At first I have nothing to jsRender - I have never used it, maybe it's OK but I think we should not use next new template system If the issue is {{}} in context of translations I think we should not use it at all in php code and use instead. The reason is that if I remember well it needs to parse all the code with eval in php so I think this is not the best way - I don't have a time to check it now but I remember I disliked {{xyz}} translations because of something and wanted to remove them at all.

The rest seems to be OK.

@kojoty I did not know that you are against existing translation marking in templates. IMHO {{label}} looks better in HTML code than <?= tr('label'); ?>, but that is because I've worked with people who did not want to have anything in common with server-side languages like PHP or Java, but were fluent in HTML, CSS, JavaScript etc. Anyway, if you are against introducing jsRender, I can make a subtemplate containing handlebars templates, include it in main template and get rid of jsRender. Subtemplates are not evaluated in context of translation marks. I tried to avoid unnecessary file multpilication, hence the idea to keep it all in one file with using another js templating library, because I didn't find anywhere in code a solution to keep handlebars code and translation markings in one main template. Described above update should be ready soon in this PR.

[rapotek]

I have implemented changes described above and updated the PR description.

[kojoty]

@rapotek , napiszę po polsku

IMHO {{label}} looks better in HTML code than <?= tr('label'); ?>

ja się zgadzam, że krótsza składania jest lepsza tylko problem w tym jakim kosztem to uzyskujemy - w przypadku OC użycie klamr {} wymaga recznego parsowania kodu każdej strony i podmieniania stringów i tego nie robi zoptymalizowany interpreter php tylko nasz kod tutaj: https://github.com/opencaching/opencaching-pl/blob/6ebc32d52d3736331b72c2e6becd17a11f2a001d/lib/common_tpl_funcs.php#L63

i później musi być eval https://github.com/opencaching/opencaching-pl/blob/6ebc32d52d3736331b72c2e6becd17a11f2a001d/lib/common_tpl_funcs.php#L182

ogólnie ten sposób mi się nie podoba i wydaje mi się "mało optymalny" - to jest narzut nakładany na kazdy widok przez nas wyświetlany.

Uważam, że warto ten twór oc-owy sluzacy do generowania templatów zastąpić czystym php - nie mozna tego zrobic o ile mamy jeszcze stare templaty uzywające {} i czasem {{}} chyba jako zmiennych - przynajmniej nie zupełnie.

...i to jest powód dlaczego nie lubię tych klamr...

Chętnie bym pokminił czy jest jakis prostszy składniowo sposób na wprowadzenie translacji w czystym php i bez eval() ale jakos na razie nie wpadłem na inne rozwiązanie niż <?=tr()?> - może masz jakiś pomysł?

[rapotek]

@rapotek , napiszę po polsku

IMHO {{label}} looks better in HTML code than <?= tr('label'); ?>

ja się zgadzam, że krótsza składania jest lepsza tylko problem w tym jakim kosztem to uzyskujemy - w przypadku OC użycie klamr {} wymaga recznego parsowania kodu każdej strony i podmieniania stringów i tego nie robi zoptymalizowany interpreter php tylko nasz kod tutaj:

https://github.com/opencaching/opencaching-pl/blob/6ebc32d52d3736331b72c2e6becd17a11f2a001d/lib/common_tpl_funcs.php#L63

i później musi być eval

https://github.com/opencaching/opencaching-pl/blob/6ebc32d52d3736331b72c2e6becd17a11f2a001d/lib/common_tpl_funcs.php#L182

ogólnie ten sposób mi się nie podoba i wydaje mi się "mało optymalny" - to jest narzut nakładany na kazdy widok przez nas wyświetlany.

Uważam, że warto ten twór oc-owy sluzacy do generowania templatów zastąpić czystym php - nie mozna tego zrobic o ile mamy jeszcze stare templaty uzywające {} i czasem {{}} chyba jako zmiennych - przynajmniej nie zupełnie.

...i to jest powód dlaczego nie lubię tych klamr...

Chętnie bym pokminił czy jest jakis prostszy składniowo sposób na wprowadzenie translacji w czystym php i bez eval() ale jakos na razie nie wpadłem na inne rozwiązanie niż <?=tr()?> - może masz jakiś pomysł?

Też oglądałem ten kod i też nie jestem nim zachwycony. Problem w tym, że nawet bardziej profesjonalne systemy szablonowe bazują na parsowaniu i podstawianiu. To prawda, eval nie jest fajny i przydałoby się tu coś innego, ale tak na szybko wydaje mi się, że zamiast się skupiać na eliminacji eval warto pójść w stronę czegoś w rodzaju "postaci półskompilowanej", tzn. przepuszczać szablon przez translację i keszować wynik dla danego języka jako skrypt php przeznaczony do includowania . Ponowne utworzenie "postaci półskompilowanej" byłoby przy modyfikacji plików językowych lub źródłowego szablonu, co łatwo wykryć. Taki wstępnie przetłumaczony szablon byłby potem gotowy do wielokrotnego wykonania reszty kodu, która w nim jest. To taki pomysł na szybko.

[deg-pl]

Ja tam jestem za zastosowaniem czegoś, co jest po prostu profesjonalnym silnikiem template'ów. Po co odkrywać Amerykę od nowa, jeśli są gotowe, dobre rozwiązania? Według mnie zdecydowanie najciekawszym jest Twig. W wersji mobile używamy Smarty, ale Smarty jest sporo słabszy. Silnik template'ów wymusza poniekąd pisanie lepszego jakościowo kodu. Template to template - nie ma tu miejsca na kod php. A w naszym kodzie w template'ach są nawet zapytania SQL...

[deg-pl]

I have implemented changes described above and updated the PR description.

@kojoty can we merge this PR? Discusson about template system we can continue elsewhere.

[andrixnet]

If there are significant changes to the template system please have them documented before merge. Thank you.

[rapotek]

If there are significant changes to the template system please have them documented before merge. Thank you.

There are no changes to the template system in this PR. Possible changes are only discussed for future.

[kojoty]

@kojoty can we merge this PR?

Sure, please go ahead.

[kojoty]

Ja tam jestem za zastosowaniem czegoś, co jest po prostu profesjonalnym silnikiem template'ów. Po co odkrywać Amerykę od nowa, jeśli są gotowe, dobre rozwiązania? Według mnie zdecydowanie najciekawszym jest Twig. W wersji mobile używamy Smarty, ale Smarty jest sporo słabszy.

A ja właśnie jestem przeciw. O ile nie robimy cechowania to moim zdaniem te phpowe silniki działające po stronie serwera są bez sensu - to tylko utrudnia dev. - a głupio robić to można zawsze i żadne tech. rozwiązanie tego nie naprawi - w sensie jak ktoś nie kuma gdzie jest miejsce na zapytania SQL to i template tu nie pomoże.

Czemu tak sądzę: taki system templatow nie wnosi nic a jest kosztem bo przetwarzanie tego jest mniej optymalne nic czystego PHP.

Popatrzcie tu: https://stackoverflow.com/questions/9363215/pure-php-html-views-vs-template-engines-views

Ja uważam, że dla nas najlepszy jest czysty PHP + cache APC na wyniki dużych zapytań do db. Zachowanie całych widoków moim zdaniem słabo się sprawdzi gdy mamy bardzo rozbudowane widoki z dużą ilością dynamicznych danych. Templaty po stronie js mają sens bo to już nie obciąża serwera bezpośrednio.

Poza wszystkim zdaje sobie sprawę że to trochę flame war...

[deg-pl]

Hmm... Jeśli dobrze pamiętam, to Ty (@kojoty) wprowadziłeś system template'ów do JS, podczas gdy przecież można używać czystego JS. Sam kiedyś używałem czystego JS do sklejania HTMLa z danymi z API i to był koszmar. Teraz do frontu używam Vue (SFC) i kod jest niesamowicie przejrzysty.

Co do systemu template'ów PHP - ja widzę trzy podstawowe zalety:

• cache'owanie template'ów. To zupełnie coś innego niż wrzucanie zapytań SQL do APCu
• wymuszenie poprawności wzorca architektonicznego. System template'ów nie pozwala na wrzucenie kodu PHP, podczas gdy w "obecnym" mamy nawet zapytania do DB. Kiedy zacząłem używać Symfony przeszkadzała mi jego sztywność - brak dostępu do bazy danych w kontrolerze (a nawet w entity). Teraz uważam, że to jedna z największych zalet - jeśli czegoś nie da się łatwo zrobić, prawdopodobnie nie powinieneś tego robić, by nie złamać architektury i logiki. Kod OC jest tak paskudny pod kątem architektury, że mnie za każdym razem szlag trafia gdy trzeba coś zmienić. Pewnie całkiem sporo jest osób, które pomogłyby rozwijać kod, gdyby ten kod jakośkolwiek normalnie wyglądał
• dostęp do narzędzi jakie daje system. Można napisać swoje, tylko po co, jeśli są gotowe?

Generalnie myślę, że OC wcześniej czy później upadnie. I to właśnie dlatego, że nie znajdzie się nikt, kto zrówna kod z ziemią i nie użyje nowoczesnych narzędzi tworząc kod od zera. Ten kod już umarł, a my go tylko czasem łatamy.

[rapotek]

I created an issue #2356 for further discussion about templating system. Feel free to fill up that issue with above comments. I suggest to discontinue this subject in this PR.