Skip to content

Conversation

@Virv12
Copy link
Contributor

@Virv12 Virv12 commented Nov 3, 2025

Closes #1544.
As already discussed the PrintingService is unused and even broken.
This PR removes all (I hope) the relative code, this makes installation easier since it requires fewer dependencies.
I just left the translations marking them as deprecated (should we completely remove them?).

Copy link
Contributor

@veluca93 veluca93 left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me but if @prandla can give it a second look I would not mind :-)

@veluca93
Copy link
Contributor

veluca93 commented Nov 3, 2025

On translations, I think we can get rid of them.

@Virv12 Virv12 force-pushed the printing branch 2 times, most recently from dba424a to 30541a1 Compare November 3, 2025 16:00
@prandla
Copy link
Member

prandla commented Nov 3, 2025

I thought we wanted to do a poll of existing users to see if they still use some of these "deprecated" features? printing was only broken in v1.5.0, it seems likely that some people might not have upgraded to it yet. and it's not hard to keep it working if someone does use it. (this feature is much less complicated than tokens for example...)

regarding the pr: i'll need to pull translation updates from weblate first to avoid merge conflicts.

i think cmscommon/tex.py is also useless now.

@prandla
Copy link
Member

prandla commented Nov 3, 2025

also, i think your updater is buggy: you're mutating self.objs while iterating over it.

@veluca93
Copy link
Contributor

veluca93 commented Nov 3, 2025

I thought we wanted to do a poll of existing users to see if they still use some of these "deprecated" features? printing was only broken in v1.5.0, it seems likely that some people might not have upgraded to it yet. and it's not hard to keep it working if someone does use it. (this feature is much less complicated than tokens for example...)

I don't think we need a poll for PrintingService.

@wil93
Copy link
Member

wil93 commented Nov 4, 2025

I could see how some contest admins might want CMS to offer this functionality, instead of having to configure it correctly. Especially if the contestant machines aren't super uniform (e.g. multiple operating systems). DOMJudge offers this functionality as well, we could ask the developers if they have an opinion on this feature and if they have plans to deprecate it.

@veluca93
Copy link
Contributor

veluca93 commented Nov 4, 2025

We have had the issue about printing being broken for two months. On that issue, the only messages present have been from people saying that we should remove it. I think that was long enough time for someone to come forward if need be (in fact, it was probably more, since the feature was broken before).

Given the cirumstances, I will merge this PR once it is technically ready. If someone complains once the feature is removed, we can restore it easily since we have access to git history.

@wil93
Copy link
Member

wil93 commented Nov 4, 2025

Fair, I guess it could even be re-implemented in an simpler way, if we'll want this in the future.

@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.66%. Comparing base (feb9906) to head (d6a1a74).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
cmscontrib/updaters/update_47.py 85.71% 2 Missing ⚠️
cms/db/user.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1583      +/-   ##
==========================================
+ Coverage   54.56%   54.66%   +0.10%     
==========================================
  Files         339      335       -4     
  Lines       27653    27358     -295     
==========================================
- Hits        15088    14956     -132     
+ Misses      12565    12402     -163     
Flag Coverage Δ
functionaltests 0.00% <0.00%> (ø)
unittests 54.66% <83.33%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Virv12
Copy link
Contributor Author

Virv12 commented Nov 4, 2025

also, i think your updater is buggy: you're mutating self.objs while iterating over it.

You are right, I thought python was smarter, now fixed.

i think cmscommon/tex.py is also useless now.

It seems you are right, removed.

Copy link
Member

@prandla prandla left a comment

Choose a reason for hiding this comment

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

libcups should also be removed from Dockerfile.

I updated the translations; you should be able to rerun the babel extract to resolve the merge conflicts.

@Virv12
Copy link
Contributor Author

Virv12 commented Nov 9, 2025

I updated the translations; you should be able to rerun the babel extract to resolve the merge conflicts.

Thanks, conflicts resolved.

I also updated the Dockerfile and the installation instruction removing the pointed out packages.

@prandla prandla merged commit 7e94024 into cms-dev:main Nov 10, 2025
4 checks passed
@Virv12 Virv12 deleted the printing branch November 10, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Printing feature has bitrotted

4 participants