Skip to content

Remove most XML responses#292

Merged
bagedevimo merged 4 commits intomasterfrom
formats-and-routes
May 18, 2025
Merged

Remove most XML responses#292
bagedevimo merged 4 commits intomasterfrom
formats-and-routes

Conversation

@bagedevimo
Copy link
Contributor

Almost none of these were being used usefully (as checked by a quick
grep of the logs on the server) except for SubmissionController#show,
which I have left in place for now. I also removed all comments refering
to routes, as a great number of them are incorrect and we discussed
briefly on discord and decided to remove. (use bundle exec rake routes
instead. Additionally, a fixed a couple of cases of record#destory being
called instead of record#destroy!. You pretty much always want to use
destroy! - the former version has no errors or warnings if the delete
fails for any reason, which is rarely what you want.

@coveralls
Copy link

coveralls commented Apr 24, 2025

Coverage Status

coverage: 37.743% (-1.0%) from 38.771%
when pulling 8a3e0ff on formats-and-routes
into be26406 on master.

@bagedevimo bagedevimo marked this pull request as ready for review April 24, 2025 22:19
Copy link
Contributor Author

bagedevimo commented May 9, 2025

@bagedevimo bagedevimo changed the base branch from master to graphite-base/292 May 9, 2025 20:19
@bagedevimo bagedevimo force-pushed the formats-and-routes branch from 709b2fd to 61b8521 Compare May 9, 2025 20:19
@bagedevimo bagedevimo changed the base branch from graphite-base/292 to write-out-fixtures-spec-helper May 9, 2025 20:19
@bagedevimo bagedevimo changed the base branch from write-out-fixtures-spec-helper to graphite-base/292 May 9, 2025 20:26
@bagedevimo bagedevimo force-pushed the graphite-base/292 branch from 1875470 to b7ca8de Compare May 9, 2025 20:26
@bagedevimo bagedevimo force-pushed the formats-and-routes branch from 61b8521 to 76519ae Compare May 9, 2025 20:26
@bagedevimo bagedevimo changed the base branch from graphite-base/292 to seeds May 9, 2025 20:26
@bagedevimo bagedevimo changed the base branch from seeds to graphite-base/292 May 9, 2025 21:39
@bagedevimo bagedevimo force-pushed the formats-and-routes branch from 76519ae to ea518a6 Compare May 9, 2025 21:39
@graphite-app graphite-app bot changed the base branch from graphite-base/292 to master May 9, 2025 21:40
@bagedevimo bagedevimo force-pushed the formats-and-routes branch from ea518a6 to 2a598e6 Compare May 9, 2025 21:40
Comment on lines +31 to +34
if params.key?(:debug) && current_user.is_admin?
render xml: @submission
else
render :show
Copy link
Member

Choose a reason for hiding this comment

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

If we're keeping this, I'd prefer keeping the .xml syntax if possible rather than changing to ?debug, since it's clearer about the response format and makes it easier for other formats to be added in the future (e.g. JSON). Potentially either of these?

Suggested change
if params.key?(:debug) && current_user.is_admin?
render xml: @submission
else
render :show
respond_to do |format|
format.html # show.html.erb
format.xml { render xml: @submission } if current_user.is_admin?
end
Suggested change
if params.key?(:debug) && current_user.is_admin?
render xml: @submission
else
render :show
if request.format.xml? && current_user.is_admin?
render xml: @submission
else
render :show

Otherwise I don't mind if we just remove the XML response from here as well for now (I think the cases you found in the logs were probably just me testing that from when you originally asked about this).

Almost none of these were being used usefully (as checked by a quick
grep of the logs on the server) except for SubmissionController#show,
which I have left in place for now. I also removed all comments refering
to routes, as a great number of them are incorrect and we discussed
briefly on discord and decided to remove. (use `bundle exec rake routes`
instead. Additionally, a fixed a couple of cases of record#destory being
called instead of record#destroy!. You pretty much always want to use
destroy! - the former version has no errors or warnings if the delete
fails for any reason, which is rarely what you want.
Admin users can add `?debug` to the URL to get the XML dump of the
specific submission, which can be useful for debugging certain issues
with judging etc.

This replaces using `.xml` to request a different format, and means we
don't need to be careful about restricting formats based on the logged
in user - instead, we can decide what we want to expose irrespective of
format, and rely on the permission model instead.

Longer term - it might be a better idea to add a "debug" tab to the
submission view for admin users, but this will do for now.
We no longer need these as we're no longer varying any content by
format.
Per a discussion in github [1] and a follow up in Discord [2] no-one
seems to need this, and it's trivial to restore later if we do.

[1] #292 (comment)
[2] https://discord.com/channels/670126531489824788/678154623571329025/1371774374872354898
@bagedevimo bagedevimo force-pushed the formats-and-routes branch from 2a598e6 to 8a3e0ff Compare May 16, 2025 23:06
@bagedevimo bagedevimo merged commit 0e0fec6 into master May 18, 2025
7 checks passed
Copy link
Contributor Author

Merge activity

@bagedevimo bagedevimo deleted the formats-and-routes branch May 18, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants