Feature/improved guild logging #2185

jezztify · 2017-06-02T18:02:05Z

Addressed Issue(s): N/A

Server Mode: Both

Description of Pull Request: To improve the Guild Activity Logging of rAthena.
It works for the following Guild Logging activities:

Guild Creation
Guild Breakage
Guild Member Addition
Guild Member Removal/Leave
Guild Alliance Formation
Guild Alliance Breakage
Guild Antagonist Formation
Guild Antagonist Breakage
Guild Castle Occupancy
Guild Castle Abandonment

Source Modification + Database Modification

…ure/Improved_Guild_Logging

secretdataz

Upgrade file still missing.
Style changes could be removed for minimal diff.
Revert the function header change.

secretdataz · 2017-06-03T03:16:01Z

sql-files/logs.sql

+	`target` VARCHAR(255) NULL DEFAULT NULL,
+	`castle_id` INT(11) NULL DEFAULT NULL
+)
+COLLATE='latin1_swedish_ci'


Remove this.

secretdataz · 2017-06-03T03:16:01Z

sql-files/logs.sql

+
+CREATE TABLE IF NOT EXISTS `interlog` (
+	`time` DATETIME NULL DEFAULT NULL,
+	`activity` VARCHAR(255) NULL DEFAULT NULL,


Could be an enum.

How about existing logs? Should we preserve them for the upgrades part of the sql?

secretdataz · 2017-06-03T03:16:01Z

src/char/inter.c

@@ -815,18 +815,14 @@ static int inter_config_read(const char* cfgName)
 }

 // Save interlog into sql
-int inter_log(char* fmt, ...)
+int inter_log(char* activity, char* origin, char* target, struct guild *g, int castle_id)


Use the va_list instead of changing the function header.

secretdataz · 2017-06-03T03:16:01Z

src/char/inter.h

@@ -26,7 +26,7 @@ void mapif_accinfo_ack(bool success, int map_fd, int u_fd, int u_aid, int accoun
 	int group_id, int logincount, int state, const char *email, const char *last_ip, const char *lastlogin,
 	const char *birthdate, const char *user_pass, const char *pincode, const char *userid);

-int inter_log(char *fmt,...);
+int inter_log(char* activity, char* initiator, char* guild_member, struct guild *g, int castle_id);


Use the va_list instead of changing the function header.

@secretdataz

…ged activity column to enum type. Used va_list in inter_log function. Reverted interlog header but changed to receive int ArgNum to count passed arguments. Thanks @secretdataz

aleos89 · 2017-06-03T22:15:05Z

src/char/int_guild.c

 			}
 		}
 	}

 	// Send on all map the new alliance/opposition
 	mapif_guild_alliance(guild_id1,guild_id2,account_id1,account_id2,flag,g[0]->name,g[1]->name);
-


Can you add this space back since it's not related to the PR?

aleos89 · 2017-06-03T22:15:05Z

src/char/int_guild.c

@@ -1760,7 +1774,6 @@ int mapif_parse_GuildCastleDataLoad(int fd, int len, int *castle_ids)
 int mapif_parse_GuildCastleDataSave(int fd, int castle_id, int index, int value)
 {
 	struct guild_castle *gc = inter_guildcastle_fromsql(castle_id);
-


Can you add this space back since it's not related to the PR?

aleos89 · 2017-06-03T22:15:05Z

src/char/inter.c

-int inter_log(char* fmt, ...)
-{
+// order of passed args:
+// activity, origin, target, guild pointer, castle_id


/** * Save interlog values to SQL * @param ArgNum: Number of arguments * Supplied arguments: activity, origin, target, guild pointer, castle_id * @return 0 */

aleos89 · 2017-06-03T22:15:05Z

src/char/inter.c

+	char* target;
+	int gid = 0;
+	int castle_id = 0;
+	int i;


You could line all these up so each one doesn't have it's own declaration line. IE:

char *activity, *origin, *target; int gid = 0, castle_id = 0, i;

Jeybla · 2017-06-04T06:20:59Z

Looks like you want to move the inter_log from sql-files/main.sql to sql-files/logs.sql.
But you're still using the main sql-handle: sql_handle. Afaik logmysql_handle is only available for the map-server, so you may want to keep the definition of the inter_log table in sql-files/main.sql .

jezztify · 2017-06-04T07:25:37Z

@Jeybla noted on that. I will put it back. I wasn't aware of such thing. Thanks :)

aleos89 · 2017-06-05T14:10:08Z

sql-files/upgrades/upgrade_20170603_interlog.sql

+	`target` VARCHAR(255) NULL DEFAULT NULL,
+	`castle_id` INT(11) NULL DEFAULT NULL
+)
+ENGINE=MyISAM;


Don't forget to add a newline here at the end of the file.

aleos89 · 2017-06-05T14:10:08Z

sql-files/logs.sql

@@ -224,4 +224,4 @@ CREATE TABLE IF NOT EXISTS `zenylog` (
  `map` varchar(11) NOT NULL default '',
  PRIMARY KEY  (`id`),
  INDEX (`type`)
-) ENGINE=MyISAM AUTO_INCREMENT=1;
+) ENGINE=MyISAM AUTO_INCREMENT=1;


Don't forget to add a newline here at the end of the file.

aleos89 · 2017-06-05T14:10:08Z

sql-files/upgrades/upgrade_20170603_interlog.sql

+	`target` VARCHAR(255) NULL DEFAULT NULL,
+	`castle_id` INT(11) NULL DEFAULT NULL
+)
+ENGINE=MyISAM;


Don't forget to add a newline here at the end of the file.

secretdataz · 2017-06-05T14:17:38Z

src/char/inter.c

+	int gid = 0, castle_id = 0, i;
+
+	va_start(ap, ArgNum);
+	for (i = 0; i < ArgNum; i++) {


Unnecessary for-loop. Just do

a = va_arg(ap, type); b = va_arg(ap, type); ... x = va_arg(ap, type);

secretdataz · 2017-06-05T14:17:38Z

src/char/inter.h

@@ -26,7 +26,7 @@ void mapif_accinfo_ack(bool success, int map_fd, int u_fd, int u_aid, int accoun
 	int group_id, int logincount, int state, const char *email, const char *last_ip, const char *lastlogin,
 	const char *birthdate, const char *user_pass, const char *pincode, const char *userid);

-int inter_log(char *fmt,...);
+int inter_log(int ArgNum, ...);


ArgNum is not needed.

@aleos89

…ted back to original inter_log header and va_list handling. Thanks @aleos89 @secretdataz

aleos89 · 2017-06-16T14:12:28Z

src/char/inter.c

@@ -820,32 +820,18 @@ static int inter_config_read(const char* cfgName)
 *  Supplied arguments: activity, origin, target, guild pointer, castle_id


The line above this, you will need to adjust the comment now from:

@param ArgNum: Number of arguments

to

@param fmt: Pointer containing the type of activity to be stored

@aleos89

Thanks @aleos89

aleos89 · 2017-07-25T22:31:01Z

src/char/inter.c

+* @param fmt: Pointer containing the type of activity to be stored
+*  Supplied arguments: activity, origin, target, guild pointer, castle_id
+* @return 0
+*/
 int inter_log(char* fmt, ...)


I'm thinking that if we always pass the same amount of arguments over to the log function, why are we using va_arg? It would be best to just define all the parameters out.

Because a lot of code in *athena wrote by people who wish just show their skills in coding and show how they are good, without thinking that others must read their codes and continue development. Good code = short, and easy to understand, well documented, or self-documented.

@aleos89 that's actually one of the reasons why I wanted to explicitly pass the values in that function and another thing is that, we're using SQL which is rigid.

Right. Whenever you have the time and want to update the function to reflect the changes we can start the process of getting this merged into master. 👍

alright. I'll find some time. THank you. (y)

Akkarinage · 2017-10-06T10:45:03Z

It'd be great if we can get the ball rolling again on this PR - @jezznar do you have any updates for this in your local branch?

jezztify added 5 commits Jun 2, 2017

WIP. Initial changes

a793fc3

WIP: SQL Files. Moved interlog to logs.sql. Modified interlog structure.

7da91c9

Merge branch 'master' of https://github.com/rathena/rathena into feat…

b0f8323

…ure/Improved_Guild_Logging

Ready for PR.

0073de9

Fixed 64bit build errors

8ccf7c0

secretdataz requested changes Jun 3, 2017

View changes

jezztify added 2 commits Jun 3, 2017

Added sql upgrade file. Removed COLLATE line from logs sql file. Chan…

3f9c356

…ged activity column to enum type. Used va_list in inter_log function. Reverted interlog header but changed to receive int ArgNum to count passed arguments. Thanks @secretdataz

Fixed c99 build error.

863ac08

aleos89 requested changes Jun 3, 2017

View changes

Changes due to Code-Review. Thanks @aleos.

3109693

moved interlog sql table creation back to main.sql

71c35d5

aleos89 approved these changes Jun 5, 2017

View changes

aleos89 requested changes Jun 5, 2017

View changes

secretdataz requested changes Jun 5, 2017

View changes

Added space at the end of interlog table creation in sql files. Rever…

17b9386

…ted back to original inter_log header and va_list handling. Thanks @aleos89 @secretdataz

aleos89 requested changes Jun 16, 2017

View changes

Updated inter_log block comment

77c4b12

Thanks @aleos89

aleos89 approved these changes Jun 16, 2017

View changes

aleos89 requested changes Jul 25, 2017

View changes

rathena / rathena Public

Feature/improved guild logging #2185

Feature/improved guild logging #2185

jezztify commented Jun 2, 2017

secretdataz left a comment

secretdataz Jun 3, 2017

secretdataz Jun 3, 2017

jezztify Jun 3, 2017

secretdataz Jun 3, 2017

secretdataz Jun 3, 2017

aleos89 Jun 3, 2017

aleos89 Jun 3, 2017

aleos89 Jun 3, 2017

aleos89 Jun 3, 2017

Jeybla commented Jun 4, 2017

jezztify commented Jun 4, 2017

aleos89 Jun 5, 2017

aleos89 Jun 5, 2017

aleos89 Jun 5, 2017

secretdataz Jun 5, 2017

secretdataz Jun 5, 2017

aleos89 Jun 16, 2017

aleos89 Jul 25, 2017

anacondaq Jul 25, 2017 •

edited

jezztify Jul 26, 2017

aleos89 Jul 26, 2017

jezztify Jul 27, 2017

Akkarinage commented Oct 6, 2017

		@@ -820,32 +820,18 @@ static int inter_config_read(const char* cfgName)
		* Supplied arguments: activity, origin, target, guild pointer, castle_id

rathena / rathena Public

Feature/improved guild logging #2185

Are you sure you want to change the base?

Feature/improved guild logging #2185

Conversation

jezztify commented Jun 2, 2017

secretdataz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jeybla commented Jun 4, 2017

jezztify commented Jun 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anacondaq Jul 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akkarinage commented Oct 6, 2017

anacondaq Jul 25, 2017 •

edited