Refactor/architecture cleanup - #11
Open
cmaureir wants to merge 12 commits into
Open
Conversation
Covers every comandos/*.py cog plus utils.py and configuration.py (103 tests), using lightweight fakes for discord.py objects instead of a full test harness like dpytest: - conftest.py (root): sandboxes a throwaway config.toml + logs/ dir and chdir()s into it before any comandos module is imported, since every such module instantiates the Config() singleton at import time and config.toml is git-ignored (real secrets). - tests/factories.py: fakes for members/roles/channels/messages/ attachments/bots. Uses MagicMock(spec=...) specifically where production code runs isinstance() checks (discord.Member, discord.TextChannel, discord.Interaction), plain objects elsewhere. - tests/conftest.py: fixtures wiring FloodSpam/Moderacion cogs the way on_ready() would, without needing a real discord.Client/Guild. - .github/workflows/tests.yml: runs the suite on push to main and on every pull request. Along the way this surfaced a few pre-existing bugs, documented as tests/comments rather than fixed here (out of scope for "add tests"): - archivar.archivar_canal() returns (False, None) on a write failure; since a non-empty tuple is always truthy, archivar()'s `if status:` treats that as success (test_failure_tuple_is_still_truthy). - FloodSpam.on_message re-fetches the channel via bot.get_channel(message.channel.id) instead of using message.channel directly; a cache miss makes self._msg_channel None and crashes the first .send() call downstream. - Moderacion._aceptar_mensaje's jump_url is built from self._msg_id, a single field shared across all pending submissions, rather than the specific post being accepted (vp.post_id) - a second submission arriving before the first is moderated could produce a jump_url pointing at the wrong message.
…gging - bot.py: register cogs from a COGS tuple instead of 7 hand-written add_cog() calls - adding a cog is now "add a class to the tuple" instead of touching both an import block and a registration block. - bot.py: guard the asyncio.run(main()) entry point with `if __name__ == "__main__":` - importing the module (e.g. from a test, or a REPL) no longer connects to Discord as a side effect. - Replace every print()/print(f"LOG: ...") call across bot.py, configuration.py, and comandos/*.py with logging.getLogger(__name__) calls at an appropriate level (debug for trace-only messages, info for state changes, warning/error/exception for actual problems). discord.utils.setup_logging() in bot.py configures the root logger by default, so these now actually reach bot.log instead of only ever appearing on stdout. - limpia.py: drop a leftover print(dir(...)) debug dump that had no operational value even as a log line.
…deracion jump_url - archivar.archivar_canal() now always returns a (success, filename) tuple, including on the unsupported-channel-type path (previously a bare None there, inconsistent with the (False, None) exception path). archivar() unpacks status/filename explicitly instead of treating the whole tuple as truthy, so a write failure - which returns the non-empty-but-failed (False, None) tuple - is no longer mistaken for success and no longer crashes trying to attach a file that was never written. - FloodSpam.on_message now sets self._msg_channel = message.channel directly instead of re-fetching it via self.bot.get_channel(message.channel.id). The refetch offered no benefit and turned a cache miss into a None channel that crashed the first .send() call downstream. - Moderacion._aceptar_mensaje now sends to the destination channel first and links to that message's own .jump_url, instead of building a URL from self._msg_id - a field holding the *original submission's* id in a different channel, mutated by every incoming submission, so a second one arriving before the first was moderated could point the link at the wrong message entirely. Updated/added tests in tests/test_archivar.py, tests/test_flood.py, and tests/test_moderacion.py to match the corrected behavior and guard against regressions.
Messages were stored as base64, but via f"{base64.b64encode(...)}" -
the repr of a bytes object (e.g. "b'aG9sYQ=='") - and reversed with
eval() to get the bytes back before b64decode. Replace with two small
helpers:
- _encode_message(): base64-encodes and stores a plain ASCII string
instead of a bytes repr.
- _decode_message(): b64decode()s that string directly. Falls back to
parsing the legacy bytes-repr format with ast.literal_eval() (not
eval()) so pending rows already logged before this change still
decode correctly instead of breaking on deploy.
attachment_check() previously called attachment.read() separately in the fast-path hash-check loop, again to cache the hash on a burst trigger, and a third time inside alert_moderation's sanitize step - up to 3 CDN round-trips per image for a single message. _hash_attachment/_sanitize_attachment (which took a discord.Attachment and read it themselves) are now _hash_bytes/_sanitize_bytes, operating on already-read bytes. attachment_check() reads each attachment once up front and reuses those bytes for hashing, caching, and (via alert_moderation's new image_bytes= parameter, replacing attachments=) sanitizing.
WARNING_COLOR (flood.py) and EMBED_COLOR (moderacion.py) were both 0x2B597B, redefined under two different names; ayuda.py and utils.py had the same value hardcoded inline with no name at all. archivar.py, limpia.py, and enviar.py each had their own one-off hex literal too. Add colors.py with named constants (BRAND, ARCHIVE, SUCCESS, BROADCAST) and use them everywhere instead, so there's one place to look up or change any embed color in the project.
…oting - flood_check (repeated messages) and attachment_check's image_burst path titled their public embeds "posible SCAM", despite being behavioral detections (repetition, image bursts) rather than the scam-link detection spam_check actually does. Retitled both to "posible SPAM", consistent with the other behavioral checks (mentions, known text/images). - known_image and image_burst's public notices were missing the "El equipo de coordinación ha sido notificado" line that every other mute-and-notify path already includes. - Replaced the repr(self._msg_content)[1:-1] quote-stripping hack in alert_moderation's "Mensaje" field with an explicit backtick escape (repr never actually escaped backticks, just stripped its own quote characters) and a "(sin texto)" placeholder for image-only messages instead of a blank code span.
FloodSpam previously threaded per-message data (channel/content/author/ author_mention) through self._msg_channel/_msg_content/_msg_author/ _msg_author_mention, set once at the top of on_message and read back by whichever check ran next. This made those methods hard to call/test independently (tests needed a prime_cog() helper to fake the setup), and - since real handling has plenty of await points in between - a second on_message() call for a *different* message running concurrently could overwrite that shared state while the first call was still relying on it. Introduce a small frozen MessageContext dataclass (message + stripped content, with channel/author/author_mention as properties) built once in on_message and passed explicitly to spam_check/flood_check/ mention_check/attachment_check/alert_moderation instead. No behavior change - same checks, same order, same messages. tests/factories.py's prime_cog() (which poked the old private attributes) is replaced with make_context(), building a real MessageContext the same way on_message does.
…loodSpam - get_moderation_channel(bot, channel_id) was a same-signature wrapper around bot.get_channel() with a name that implied more than it did (it doesn't look up "the mod channel", it returns whatever channel id it's given). Inlined at all 5 call sites, removed from utils.py. - messages.py's Messages dataclass was only ever used by FloodSpam (spam/normal/image_spam/image_authors), accessed everywhere through an extra self.messages.x layer of indirection. Folded those four fields directly onto FloodSpam as plain attributes and deleted messages.py. No behavior change - same lookups, same state, just fewer names to track for something only one cog ever used.
It compared bot.get_channel(ctx.channel.id) against ctx's own channel - always true, since ctx.channel is already resolved from that same id. It never actually validated anything; the real gate on aceptar/rechazar/mod is @commands.has_role(config.MOD_ROLE).
mensaje_ayuda's channel check (bot.get_channel(ctx.channel.id), always truthy since ctx.channel already comes from that id) made get_mod_help the only reachable branch in practice. The other branch, get_main_help, documented %encuesta - a command removed in f97c8ae. Simplified to always send the mod help and deleted the dead branch/content.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.