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

CS restore training map #874

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

CS restore training map #874

wants to merge 5 commits into from

Conversation

cs-endo
Copy link
Contributor

@cs-endo cs-endo commented Apr 25, 2020

Bear in mind this is my first time doing PHP so it can likely be adjusted. This is mostly an initial stage to see how well it works. I've tested this out locally and it seems to display everything fairly okay. Query in the main PHP file gives the expected result in our database too.

Removes old image variable and adds in "text"
Allows you to get names of all people with a certain training
Removes the old empty image and shows the query result
@cs-endo
Copy link
Contributor Author

cs-endo commented Apr 25, 2020

Actually I need to consider if the SQL query takes the right test value. This might show the names of people but not necessarily those with that training, depending on if I got it right. My test sample was 2 users with self-gained training so this could have caused that issue. All I would need to do is change the "WHERE" condition to the right value

Copy link
Member

@mstratford mstratford left a comment

Choose a reason for hiding this comment

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

This seems fair. Might be worth being called getAllTrained or something.

It may be worth looking at removing the MyRadio Daemon associated with generation of the images. In production it's disabled in the config because it typically breaks after a PHP upgrade. It uses a lot of graphics PHP packages to do it, which are often forgotten.

@mstratford
Copy link
Member

I'm torn between essentially returning all the users as MyRadio_User so you can link to them, but I also know that there's a lot of overhead there for a list of people. So I'm not too fussed.

Copy link
Member

@markspolakovs markspolakovs left a comment

Choose a reason for hiding this comment

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

This works, but per @mstratford I'm not entirely sure this is the best way of doing things - what if you, for example, want links to each person's profile?

Overhead in a list of MyRadio_User objects shouldn't be that bad, even if it's everyone studio trainer trained...

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