Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,9 @@
"command-description": "Request a Custom role as a reward for boosting. This has a cooldown of 24 hours",
"manage-subcommand-description": "Create or edit your custom role",
"name-option-description": "The name of your custom role",
"color-option-description": "The color of your custom role",
"primary-color-option-description": "The (primary) color of your custom role",
"secondary-color-option-description": "The secondary color of your custom role. (If unlocked)",
"holographic-option-description": "If enabled, your role will use the holographic effect. (If unlocked)",
"remove-subcommand-description": "Remove your custom role",
"icon-option-description": "Your role-icon",
"confirm-option-remove-description": "Do you really want to delete your custom role? This will not reset any running cooldowns"
Expand Down
96 changes: 65 additions & 31 deletions modules/color-me/commands/color-me.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const {localize} = require('../../../src/functions/localize');
const {client} = require('../../../main');
const {embedType, dateToDiscordTimestamp} = require('../../../src/functions/helpers');
const { Constants } = require('discord.js');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: const { Constants } here vs const {Constants} in guildMemberUpdate.js:2 and the rest of the codebase. @stylistic/object-curly-spacing is off so lint won't catch it.


module.exports.beforeSubcommand = async function (interaction) {
await interaction.deferReply({ephemeral: true});
Expand All @@ -10,6 +11,7 @@ module.exports.subcommands = {
'manage': async function (interaction) {
let roleIcon;
let iconW = true;
let colorfulW = true;
if (interaction.options.getAttachment('icon') !== null) {
if (client.guild.features.includes('ROLE_ICONS')) {
roleIcon = interaction.options.getAttachment('icon').url;
Expand All @@ -22,6 +24,11 @@ module.exports.subcommands = {
const moduleStrings = interaction.client.configurations['color-me']['strings'];
const moduleModel = interaction.client.models['color-me']['Role'];

const multiColor = client.guild.features.includes('ENHANCED_ROLE_COLORS');
if ((!multiColor && (interaction.options.getString('secondary-color') !== null || interaction.options.getBoolean('holographic'))) || !moduleConf['allowEnhancedRoleColors']) {
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
colorfulW = false;
}

const pos = moduleConf.rolePosition
? interaction.guild.roles.resolve(moduleConf.rolePosition).position
: 0;
Expand All @@ -44,29 +51,40 @@ module.exports.subcommands = {
userID: interaction.user.id
}
});
const {
roleColor: primaryColor,
cancel
} = await color(interaction.options.getString('primary-color'), interaction, moduleStrings);
let {
roleColor: secondaryColor,
cancel: cancelSec
} = await color(interaction.options.getString('secondary-color'), interaction, moduleStrings);
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
if (!multiColor) {
secondaryColor = null;
}
if (cancel || cancelSec) return;
const isHolographic = interaction.options.getBoolean('holographic') && multiColor;
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
if (role) {
role = role.roleID;
const {
roleColor,
cancel
} = await color(interaction, moduleStrings);
if (cancel) return;
if (interaction.guild.roles.cache.find(r => r.id === role)) {
role = interaction.guild.roles.resolve(role);
role.edit(
await role.edit(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking - role is never patched by edit(), so the write-back below saves stale values.

RoleManager#edit returns a clone and leaves the original untouched (discord.js/src/managers/RoleManager.js:295-297):

const clone = role._clone();
clone._patch(d);
return clone;

and _clone() is Object.assign(Object.create(this), this), so _patch writes a fresh colors object onto the clone only.

That means at line 96-99 role.name and role.colors are still the pre-edit values, and the row records what the role looked like before the edit - which is exactly the staleness the write-back was added to fix. (The cached role does get patched eventually by the GUILD_ROLE_UPDATE gateway event, but that's a race, not a guarantee, and it definitely hasn't landed by the next line.)

Assigning the return value fixes it:

Suggested change
await role.edit(
role = await role.edit(

{
name: interaction.options.getString('name'),
color: roleColor,
colors: isHolographic ? Constants.HolographicStyle : {
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
primaryColor: primaryColor,
secondaryColor: secondaryColor
},
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
icon: roleIcon,
reason: localize('color-me', 'edit-log-reason', {
user: interaction.user.username
})
}
);
Comment thread
hfgd123 marked this conversation as resolved.
if (iconW) {
if (iconW && colorfulW) {
await interaction.editReply(embedType(moduleStrings['updated'], {}));
} else {
await interaction.editReply(embedType(moduleStrings['updatedNoIcon'], {}));
await interaction.editReply(embedType(moduleStrings['updatedLimited'], {}));
}
} else {
if (interaction.guild.roles.cache.size >= 250) {
Expand All @@ -76,7 +94,10 @@ module.exports.subcommands = {
role = await interaction.guild.roles.create(
{
name: interaction.options.getString('name'),
color: roleColor,
colors: isHolographic ? Constants.HolographicStyle : {
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
primaryColor: primaryColor,
secondaryColor: secondaryColor
},
icon: roleIcon,
hoist: moduleConf.listRoles,
permissions: '',
Expand All @@ -85,13 +106,14 @@ module.exports.subcommands = {
reason: localize('color-me', 'create-log-reason', {
user: interaction.user.username
})
}
);
});
await moduleModel.update({
userID: interaction.user.id,
roleID: role.id,
name: role.name,
color: role.hexColor,
primaryColor: role.colors.primaryColor,
secondaryColor: role.colors.secondaryColor,
holo: !!role.colors.tertiaryColor,
Comment on lines +140 to +142

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same integer-vs-hex problem as line 97 - this is the create-after-record-exists path.

timestamp: new Date()
}, {
where: {
Expand All @@ -101,23 +123,21 @@ module.exports.subcommands = {
if (!interaction.member.roles.cache.has(role)) {
await interaction.member.roles.add(role);
}
if (iconW) {
if (iconW && colorfulW) {
await interaction.editReply(embedType(moduleStrings['updated'], {}));
} else {
await interaction.editReply(embedType(moduleStrings['updatedNoIcon'], {}));
await interaction.editReply(embedType(moduleStrings['updatedLimited'], {}));
}
}
} else {
const {
roleColor,
cancel
} = await color(interaction, moduleStrings);
if (cancel) return;
try {
role = await interaction.guild.roles.create(
{
name: interaction.options.getString('name'),
color: roleColor,
colors: isHolographic ? Constants.HolographicStyle : {
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
primaryColor: primaryColor,
secondaryColor: secondaryColor
},
icon: roleIcon,
hoist: moduleConf.listRoles,
permissions: '',
Expand All @@ -132,11 +152,13 @@ module.exports.subcommands = {
userID: interaction.user.id,
roleID: role.id,
name: role.name,
color: role.hexColor,
primaryColor: role.colors.primaryColor,
secondaryColor: role.colors.secondaryColor,
holo: !!role.colors.tertiaryColor,
Comment on lines +185 to +187

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same integer-vs-hex problem again - the first-time create path.

timestamp: new Date()
});
await interaction.member.roles.add(role);
if (iconW) {
if (iconW && colorfulW) {
await interaction.editReply(embedType(moduleStrings['created'], {}));
} else {
await interaction.editReply(embedType(moduleStrings['createdNoIcon'], {}));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

updatedNoIcon was renamed to updatedLimited to cover both features, but the create path still falls through to createdNoIcon, whose text is unchanged:

Your role was created successfully, but your role icon was not used, as this requires the guild to be boost level 2 or higher.

So a user who asks for holographic on a non-enhanced guild is told their icon was dropped. createdNoIcon needs the same rename + rewording treatment as updatedLimited in strings.json.

Expand Down Expand Up @@ -168,7 +190,7 @@ module.exports.subcommands = {
role = role.roleID;
if (interaction.guild.roles.cache.find(r => r.id === role)) {
role = interaction.guild.roles.resolve(role);
role.delete(localize('color-me', 'delete-manual-log-reason', {
await role.delete(localize('color-me', 'delete-manual-log-reason', {
user: interaction.member.user.username
}));
await interaction.editReply(await embedType(moduleStrings['removed'], {}));
Expand Down Expand Up @@ -196,8 +218,20 @@ module.exports.config = {
{
type: 'STRING',
required: false,
name: 'color',
description: localize('color-me', 'color-option-description')
name: 'primary-color',
description: localize('color-me', 'primary-color-option-description')
},
{
type: 'STRING',
required: false,
name: 'secondary-color',
description: localize('color-me', 'secondary-color-option-description')
},
{
type: 'BOOLEAN',
required: false,
name: 'holographic',
description: localize('color-me', 'holographic-option-description')
},
{
type: 'ATTACHMENT',
Expand Down Expand Up @@ -227,9 +261,9 @@ module.exports.config = {
* Gets a color from the String of a command option
* @returns {Promise<{roleColor: string|number, cancel: boolean}>}
*/
async function color(interaction, moduleStrings) {
if (interaction.options.getString('color')) {
let roleColor = interaction.options.getString('color');
async function color(colorString, interaction, moduleStrings) {
if (colorString) {
let roleColor = colorString;
if (!roleColor.startsWith('#')) {
roleColor = '#' + roleColor;
}
Expand All @@ -246,12 +280,12 @@ async function color(interaction, moduleStrings) {
};
}
return {
roleColor: 0xF1C40F,
roleColor: 0x000000,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This flips the default from gold to colorless for every role created or edited without primary-color - and since manage always sends colors, that includes someone editing only their role name.

It's arguably the better default (Discord renders 0 as "no color" rather than actual black), but it's a silent behaviour change for every existing server, so worth being deliberate about it rather than letting it ride in as part of the gradient work.

cancel: false
};
}

// Exported for unit testing of the colour-validation logic.
// Exported for unit testing of the color-validation logic.
module.exports.color = color;

/**
Expand Down
7 changes: 7 additions & 0 deletions modules/color-me/configs/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@
"default": "",
"description": "The role, beneath which the custom-roles should be created",
"type": "roleID"
},
{
"name": "allowEnhancedRoleColors",
"humanName": "Allow \"Enhanced Role Colors\"",
"default": true,
"description": "Should the module allow users to use the \"Enhanced Role Colors\" feature? (If your server doesn't have this feature unlocked, this setting will have no effect)",
"type": "boolean"
}
Comment on lines +41 to 47

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

config-localizations/en.json needs regenerating - it's the generated Weblate reference, and at this head it still carries the old updatedNoIcon entry and has no entry for allowEnhancedRoleColors or updatedLimited.

Nothing crashes (getLocalizedConfig falls back to the inline English when a key is missing), but the new field and the new string can never be translated, and translators keep seeing a dead string.

node config-localizations/generate-files.js

]
}
8 changes: 4 additions & 4 deletions modules/color-me/configs/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
"allowEmbed": true
},
{
"name": "updatedNoIcon",
"humanName": "Role updated without icon",
"default": "Your role was updated successfully, but your role icon was not used, as this requires the guild to be boost level 2 or higher.",
"description": "This messages gets send when a booster sucessfully updates their custom role, but the guild has not enough boosts to use role icons",
"name": "updatedLimited",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Heads-up rather than a change request: renaming the key means any server that customised updatedNoIcon silently loses that text and reverts to this new default (configuration.js falls back to the field default when a stored key is absent). Fine given the meaning genuinely changed - just a one-way reset worth knowing about.

"humanName": "Role updated with limited features",
"default": "Your role was updated successfully, but either your role icon or enhanced role colors were not used, as these features either need to be unlocked by boosting the server or were disabled by the server admin.",
"description": "This messages gets send when a booster sucessfully updates their custom role, but the guild has not enough boosts to use either role icons or enhanced role colors or the feature was disabled in the module configuration",
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
"type": "string",
"allowEmbed": true
},
Expand Down
12 changes: 9 additions & 3 deletions modules/color-me/events/guildMemberUpdate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const {localize} = require('../../../src/functions/localize');
const {Constants} = require('discord.js');
let pos;

module.exports.run = async function (client, oldGuildMember, newGuildMember) {
Expand Down Expand Up @@ -36,7 +37,7 @@ module.exports.run = async function (client, oldGuildMember, newGuildMember) {
if (moduleConf.recreateRole) {
if (!oldGuildMember.premiumSince && newGuildMember.premiumSince) {
const data = await client.models['color-me']['Role'].findOne({
attributes: ['roleID', 'name', 'color'],
attributes: ['roleID', 'name', 'primaryColor', 'secondaryColor', 'holo'],
raw: true,
where: {
userID: newGuildMember.id
Expand All @@ -45,12 +46,17 @@ module.exports.run = async function (client, oldGuildMember, newGuildMember) {
if (data) {
let role = data.roleID;
const name = data.name;
const color = data.color;
const primaryColor = data.primaryColor;
const secondaryColor = data.secondaryColor;
const isHolographic = data.holo && client.guild.features.includes('ENHANCED_ROLE_COLORS');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

allowEnhancedRoleColors got folded into multiColor in color-me.js, but the event path still checks only the guild feature. So an admin who turns the setting off still gets holographic roles recreated on reboost - the switch is still half-inert.

moduleConf is already in scope from line 10, and hoisting the check into a local also cleans up the duplicate features.includes() on line 62:

Suggested change
const isHolographic = data.holo && client.guild.features.includes('ENHANCED_ROLE_COLORS');
const enhancedColors = client.guild.features.includes('ENHANCED_ROLE_COLORS') && moduleConf['allowEnhancedRoleColors'];
const isHolographic = data.holo && enhancedColors;

if (!newGuildMember.guild.roles.cache.find(r => r.id === role)) {
role = await client.guild.roles.create(
{
name: name,
color: color,
colors: isHolographic ? Constants.HolographicStyle : {
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
primaryColor: primaryColor,
secondaryColor: client.guild.features.includes('ENHANCED_ROLE_COLORS') ? secondaryColor : '#000000'
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
},
hoist: moduleConf.listRoles,
position: pos,
permissions: '',
Expand Down
37 changes: 37 additions & 0 deletions modules/color-me/migrations/colorme_Role__V1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
const {DataTypes} = require('sequelize');

const TABLE = 'colorme_Role';

module.exports = {
// Tables to snapshot before this migration runs (see Backups). Optional but recommended.
tables: [TABLE],

up: async ({context: {queryInterface, sequelize}}) => {
await sequelize.transaction(async (transaction) => {
const description = await queryInterface.describeTable(TABLE).catch(() => ({}));

if (!description.primaryColor && description.color) {
await queryInterface.renameColumn(TABLE, 'color', 'primaryColor', {transaction});
}
if (!description.secondaryColor) {
await queryInterface.addColumn(TABLE, 'secondaryColor', {
type: DataTypes.STRING
}, {transaction});
}
if (!description.holo) {
await queryInterface.addColumn(TABLE, 'holo', {
Comment thread
hfgd123 marked this conversation as resolved.
type: DataTypes.BOOLEAN
}, {transaction});
}
});
},

down: async ({context: {queryInterface, sequelize}}) => {
await sequelize.transaction(async (transaction) => {
const description = await queryInterface.describeTable(TABLE).catch(() => ({}));
if (description.primaryColor && !description.color) await queryInterface.renameColumn(TABLE, 'primaryColor', 'color', {transaction});
if (description.secondaryColor) await queryInterface.removeColumn(TABLE, 'secondaryColor', {transaction});
if (description.holo) await queryInterface.removeColumn(TABLE, 'holo', {transaction});
});
}
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: no trailing newline. (@stylistic/eol-last is off, so this is cosmetic.)

4 changes: 3 additions & 1 deletion modules/color-me/models/Role.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ module.exports = class Role extends Model {
userID: DataTypes.STRING,
roleID: DataTypes.STRING,
name: DataTypes.STRING,
color: DataTypes.STRING,
primaryColor: DataTypes.STRING,
secondaryColor: DataTypes.STRING,
holo: DataTypes.BOOLEAN,
Comment thread
hfgd123 marked this conversation as resolved.
Outdated
timestamp: DataTypes.DATE
}, {
tableName: 'colorme_Role',
Expand Down
Loading
Loading