Skip to content

Add match faction display - #18

Open
hbeni wants to merge 4 commits into
Greg-Boggs:masterfrom
hbeni:addFactionsToReport
Open

Add match faction display#18
hbeni wants to merge 4 commits into
Greg-Boggs:masterfrom
hbeni:addFactionsToReport

Conversation

@hbeni

@hbeni hbeni commented Sep 7, 2021

Copy link
Copy Markdown

This will add faction reporting capability and displays the factions in the game details.

⚠️ Attention:

  • The db schema needs to be extended (add columns 'faction1' and 'faction2' to webl_games)
  • The config needs a new global config array $factions (see default_config.php.dist, l. 126)

This will add faction reporting capability and displays the factions in the game details.

/!\ Attention:
 - The db schema needs to be extended (add columns 'faction1' and 'faction2' to webl_games)
 - The config needs a new global config array $Factions (see default_config.php.dist, l. 126)
@hbeni

hbeni commented Sep 7, 2021

Copy link
Copy Markdown
Author

I had no test bed to verify its working.
The code was lint checked, however; and i double checked the logic with my eyeballs.

Is there a test installation where this could be tested?

@Greg-Boggs

Copy link
Copy Markdown
Owner

We had a dev server when we migrated to PHP7, but we haven't been using it so I shut it down. We can just test in prod.

@Greg-Boggs

Copy link
Copy Markdown
Owner

left a couple items in the code review. Code also needs a SQL update statement to update the currently live table.

@hbeni

hbeni commented Sep 8, 2021

Copy link
Copy Markdown
Author

left a couple items in the code review

Hi, @Greg-Boggs
I tried to find them - the review is empty for me however.

Code also needs a SQL update statement to update the currently live table.

Where do i find the code part?

ping @Greg-Boggs

@Greg-Boggs Greg-Boggs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

left a couple items in the code review

Hi, @Greg-Boggs
I tried to find them - the review is empty for me however.

Code also needs a SQL update statement to update the currently live table.

Where do i find the code part?

ping @Greg-Boggs

There is no code for this. I'll need the SQL statement so I can run it to update the database that's currently running.

Comment thread report.php
Comment thread conf/default_conf.php.dist Outdated
@hbeni

hbeni commented Sep 10, 2021

Copy link
Copy Markdown
Author

Where do i find the code part?

There is no code for this. I'll need the SQL statement so I can run it to update the database that's currently running.

I was of the impression of the code, that this is not needed in this case, because the factions go into the match details, and they are displayed by the reports pages already. If you know other places I need to adjust, please let me know.
Especially here, where the $gamestable is queried for the results (which I ammended to include the factions):
https://github.com/hbeni/GamingLadder/blob/6b1aecc82b41787ddd1c98b5fb764781a994827c/gamedetails.php#L150

By skimming trough the code again, i just have seen that i missed to add the factions columns in the select (just fixed this by
eabd76b ):
https://github.com/hbeni/GamingLadder/blob/6b1aecc82b41787ddd1c98b5fb764781a994827c/gamehistory.php#L225

@hbeni

hbeni commented Sep 10, 2021

Copy link
Copy Markdown
Author

Code also needs a SQL update statement to update the currently live table.

Oh, do I maybe misunderstood, and you meant a statement to update the schema of the current life DB?
That would be:
ALTER TABLE webl_games ADD faction1 varchar(40) default NULL, ADD faction2 varchar(40) default NULL;

(The initial setup sql I already adjusted in the PR.)

@Greg-Boggs

Copy link
Copy Markdown
Owner

While you adjusted the provided SQL database, that only gets used by people who want to start a brand new website from scratch. Can you include the .sql file you want run against the database?

@hbeni

hbeni commented Sep 10, 2021

Copy link
Copy Markdown
Author

[... some review comments in the PR code ...]

I adressed them all I hope :)

While you adjusted the provided SQL database, that only gets used by people who want to start a brand new website from scratch. Can you include the .sql file you want run against the database?

I already gave the needed statement in my comment above. Is there any specific method you want me to include it?

EDIT added an update SQL file and a short readme/description (to the admin/install folder). This way, also future DB updates can be easily handled.

@hbeni
hbeni force-pushed the addFactionsToReport branch from 259d956 to ef90eb0 Compare September 10, 2021 20:09
@hbeni

hbeni commented Sep 16, 2021

Copy link
Copy Markdown
Author

Hi there, can I still do something? Is something missing still? :)

@hbeni

hbeni commented Mar 8, 2024

Copy link
Copy Markdown
Author

Anything new here?

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