Skip to content

Conversation

@pferreir
Copy link
Member

This adds:

  • A field/column converted_from (and backref converted_to) to every Attachment, which can of course be None, and will help us keep track of attachments generated from other attachments
  • An annotations field/column (JSONB), useful to add metadata such as origin of the attachment (CloudConvert? CodiMD archiver? others?)
  • A collation rule, called case_insensitive, to the database, which does case-insensitive "predictable" sorting of names which takes into account also punctuation characters (contrary to Postgres' default). This is added only because I noticed that the sorting in attachments names was unintuitive
  • A "magic wand" icon to the icomoon font, so that plugins can use it to signal generated attachments 🪄

@github-actions github-actions bot added the alembic Contains database changes label Sep 30, 2025
@pferreir pferreir force-pushed the add-attachment-annotations branch 2 times, most recently from 2af5c08 to 38c648c Compare September 30, 2025 12:46
@pferreir pferreir force-pushed the add-attachment-annotations branch 2 times, most recently from 1042d1b to da77adf Compare October 8, 2025 13:00
Copy link
Member

@ThiefMaster ThiefMaster left a comment

Choose a reason for hiding this comment

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

LGTM, but I think it's best to (test and) merge it together w/ the plugin changes

Comment on lines +78 to +80
# remove PG* variables from environment, as they may cause the following commands to fail
for env_var in ('PGUSER', 'PGPASSWORD', 'PGHOST', 'PGPORT', 'PGDATABASE'):
os.environ.pop(env_var, None)
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to discard any PG* env vars?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to be so dramatic, in case there is some legit use for some variable starting with PG. But I'm not sure.

@pferreir
Copy link
Member Author

LGTM, but I think it's best to (test and) merge it together w/ the plugin changes

Any particular reason why you would not merge this one first? Just afraid that it will become stale.

@ThiefMaster
Copy link
Member

Primarily in case anything is missing... but OTOH it looks pretty complete. I guess you tested the collation stuff with weird characters (emoji, chinese and whatever) so it doesn't fail with an error in case of such content?

@pferreir
Copy link
Member Author

I guess you tested the collation stuff with weird characters (emoji, chinese and whatever) so it doesn't fail with an error

I tried it with some special characters. Not necessarily Asian scripts.

@ThiefMaster ThiefMaster added this to the v3.3 milestone Oct 10, 2025
@ThiefMaster ThiefMaster force-pushed the add-attachment-annotations branch from a473fb1 to 611fad6 Compare October 17, 2025 13:22
I don't like to just cascade there, even though this should only be used
on test databases with no valuable data
@ThiefMaster ThiefMaster merged commit 8edee63 into indico:master Oct 17, 2025
10 checks passed
@ThiefMaster ThiefMaster deleted the add-attachment-annotations branch October 17, 2025 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alembic Contains database changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants