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

Code Consolidation for API preparations & hard coded path replacements #901

Merged
merged 21 commits into from
May 20, 2023
Merged

Code Consolidation for API preparations & hard coded path replacements #901

merged 21 commits into from
May 20, 2023

Conversation

jaredb7
Copy link

@jaredb7 jaredb7 commented May 9, 2023

Review required!!
This is my attempt at consolidating code and functionality into a common file to later be also used by a API.

No functionality has changed, just the code has moved and a bunch of helper functions written to make life easier (saveSettings(), getDirectory() and getFilePath() are some of them)
Everywhere some functionality existed that was moved into a function in common.php I've tested those pages and any actions (like protecting recordings) etc.

  • All SQL Queries were moved into their respective functions and parameterised, there were some slight logic changes in the front end pages to accommodate data being returned differently but actual data returned and functionality is maintained

  • A common function is used to execute all queries and looks after binding any parameterised values that have been supplied. We're already using prepared statement so updated all queries that would accept input from the user with place-holders which are later bound into the statement.

  • The Config and Advanced setting pages received changes to use the new setSetting() function which looks after creating or updating the setting in the required config files. It also refreshed the global $config variable by re-reading the config files after saving. Optionally it also specification of a command to run after saving the settings.

  • Service Controls page received changes with the replacement of the actual command to execute with a sort of shorthand human friendly versions e.g. service <service_name> -- service restart livestream.service.
    These commands map to the original commands in serviceMaintenance() so this could be called through the API without needing the full-blown commend.
    views.php was updated to accommodate this also

  • System Controls page received a small change to make the process checking for new git updates or getting the last commit hash a single function call

  • Two new functions getDirectory() and getFilePath() were introduced in aims and make it easier to relocate files in the future. This has been worked into all existing php files to replace all hard coded paths.
    Currently the home directory is still fixed to that of user id 1000 (normally pi) but can be changed once we get around to needed that.

  • Other misc code for deleting / protecting detection's, frequency shift, Flickr API call etc have also been moved into common.php to be used later by the API as well

  • Excuse the misleading branch name 😄

All SQL Queries were moved into their respective functions and parameterized, there were some slight logic changes in the front end pages to accommodate data being returned differently but existing functionality is maintained

A common function is used to execute the queries and looks after binding any parameterized values that have been supplied.

Other code for deleting / protecting detection's, frequency shift, Flickr API call etc have also been moved into common.php to be used later by the API

Two new functions getDirectory and getFilePath were introduced in aims and make it easier to relocate files. This will be worked into all existing php files to replace hardcoded paths
The Config and Advanced setting pages received changes to use the new setSetting() function which looks after creating or updated the setting in the required config files.
Optionally it also specification of a command to run after saving the settings.
Some variables like models, audio formats and frequency shift tools were also moved into common.php

Service Controls page received changes with the replacement of the actual command to execute with a sort of shorthand human friendly version e.g. service <action> <service_name> -- service restart livestream.service

These commands map to the original commands in serviceMaintenance() so this could be called through the API without needing the full-blown commend.
views.php was updated to accommodate this also

System Controls page received a small change to make the process checking for new git updates or getting the last commit hash a single function call.

Include and Exclude species updated to use getFilePath() to generate a path to the required file instead of the hard coded path

Views.php also received similar treatment with replacing some hard coded paths with generated paths, but logic changes to handle the change in running service commands
Install langauge code was moved into a different file, updated changeLanguage() in common.php by removing sudo from 3K model language install.
Include and Exclude list updated to use the calculated path to their respective x_species_list.txt files

Move play.php over to also use the new calculated paths to disk_check_exclude

Spectrogram.php remove unused code & replace setting save code with new function

Stats.php removed old code, update paths to use calculated path.

Rename setSetting func to saveSetting
Remove old code from other pages
# Resolve Conflicts due to code movement:
#	homepage/views.php
#	scripts/config.php
#	scripts/play.php
Wired in an additional check to verify user is authenticated before executing any code that makes changes to the system like saving settings or command execution.
This is a 2nd layer on top of the front end pages authenticating the user

Added in escaping of arguments for set date and change timezone just in case
@Svardsten53
Copy link

I have some questions about this. As I mentioned earlier, I have made some modifications to the program for myself and other users in Sweden. In index.php the title has been changed and current weather info has been added below it. In views.php I have made a Swedish translation and modification of the menu. In other pages, English has been translated into Swedish. Everything is modified after each update by running some scripts and it works very well.

I have tried to understand the new structure, but do not know PHP very well. I would like to be able to prepare a bit so that it will also work in the future.

Is common.php a page that other pages that exist today only will call as a general resource?

Will index.php, views.php and all other files that build the appearance of the program remain with the same content in the future?

… by auth prompt

We always want this command to execute regardless and rebuild the caddy config accordingly.
@jaredb7
Copy link
Author

jaredb7 commented May 10, 2023

Hi @Svardsten53 , I did see your modified version, I like the weather at in the title :), are you using a different font too?

Correct all existing pages will call common.php to execute code that was in that file but now moved into common.

The layout/html code out from index.php, views.php etc to build the appearance we see today won't change or be replaced. So your script used to do the translations will continue to work after any tweaks if necessary, the code replacing the title with weather should also be ok too, depending on how you're doing it, the page links and bird stats chart might review.

The idea was we keep the current look and feel and output untouched, moved any code that can be reused to common.php, build a API that reuses that same code, then a new web interface can build off the data from the API.

@Svardsten53
Copy link

Thanks for a detailed answer. I found the program when it was only a few weeks old, wanted to adapt it somewhat to Swedish conditions and had a lot of contact with Patrick during that time. I haven't changed the font, but probably using what was first there, it may have been changed later in the original version.

The weather data is downloaded via an API from the nearest local official weather station and unfortunately the solution cannot be used in other countries.

It sounds hopeful that the files that control the appearance won't change too much. Then it should be enough for me to make some small changes in the four or five files I modified, the rest are just translations.

Because saveSetting() checks each time to see that the user has authenticated, if the password is changed first then very next setting will cause a auth prompt (as the password changed) and if cancelled then the other setting won't save.

Putting it last will allow us to save all the other setting and then change the password, then the next attempt to execute a action or access a page that required auth will prompt the user the the password.
…r us

Makes things a little tidier and easier to change in the future, a custom message can be passed to authenticateUser() and this is output if the authentication fails.

Added protection around Include and Exclude species pages as these still accessible & data can still be written to include and exclude species.txt even with a password.
Quick test for the common.php directory and filepath resolver to ensure it's generating the correct paths

And small fix for incorrect firstrun.ini location (thanks test case :) )
@jaredb7 jaredb7 marked this pull request as ready for review May 15, 2023 13:33
@jaredb7 jaredb7 changed the title Code Consolidation for API preparations Code Consolidation for API preparations & hard coded path replacements May 15, 2023
To avoid having to update file or directory paths in 3 script locations (PHP Common, Shell Common and Python File path resolver)

The .json file will references by all 3 systems, allowing  them to generate paths from one central location. Files can be then moved around, the file updated and all 3 languages will up to date without any other work.
@ehpersonal38
Copy link
Collaborator

ehpersonal38 commented May 20, 2023

Thank you for the contribution. This is an absolute monolith of code, I'm honestly shocked and a bit scared 😄.

Anyways, there's a fatal error showing on my system when I tested this PR out -
chrome_yoQw8EKPrS

@jaredb7
Copy link
Author

jaredb7 commented May 20, 2023

It is quite a large change lol, hopefully the PHP comments made it a little less freighting haha 😅

Whoops and apologies, I'll investigate the error, I think it may be because it can't find the new config directory with the new json file which I've probably linked onto /homepage on my install but haven't added it into update_snippets

@jaredb7
Copy link
Author

jaredb7 commented May 20, 2023

It was the missing symlink :(

[ -L ~/BirdSongs/Extracted/config ] || ln -sf ~/BirdNET-Pi/config ~/BirdSongs/Extracted

I committed the change to the same branch and opened a new PR, I'm sure there some magic you're able to do to revert the revert & pull in the fix :)

Let me know if something different has to be done.

@ehpersonal38
Copy link
Collaborator

Are you able to re-open this PR by chance?

@jaredb7
Copy link
Author

jaredb7 commented May 22, 2023

image
Hmm, I don't seem to have the option anywhere, I do have a option to create a new PR from the revert but that doesn't work since the branch was deleted and probably not a good idea nyway

Would it be beneficial in creating a new PR, with the fix from #919 and resolve any conflicts then submit it against PR-Development and see how that looks?

@ehpersonal38
Copy link
Collaborator

Would it be beneficial in creating a new PR, with the fix from #919 and resolve any conflicts then submit it against PR-Development and see how that looks?

Yep, let's try that!

@jaredb7
Copy link
Author

jaredb7 commented May 24, 2023

New PR #925 submitted against the PR Dev branch, I pulled in the latest commits to the main branch, made adjustments and resolved the conflicts that were occurring.
This also has #919 in it some small changes

Then I compared the files again what I have been running for the past few weeks just to see what I may have to re-test and there were no odd changes after resolving things.

I tested the new frequency shift option on the spectrogram page and also the 30Day graphs fix and fixed the issue my code created (wasn't supplying valid data)

Let me know if there are any issues :)

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.

None yet

3 participants