Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #1159 #1164

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Fix issue #1159 #1164

merged 1 commit into from
Mar 10, 2021

Conversation

ranaldmiao
Copy link
Contributor

@ranaldmiao ranaldmiao commented Nov 8, 2020

This PR fixes issue #1159


This change is Reviewable

@codecov
Copy link

codecov bot commented Nov 8, 2020

Codecov Report

Merging #1164 (b920fe1) into master (9236084) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1164      +/-   ##
==========================================
- Coverage   63.63%   63.58%   -0.05%     
==========================================
  Files         232      232              
  Lines       17044    17044              
==========================================
- Hits        10846    10838       -8     
- Misses       6198     6206       +8     
Flag Coverage Δ
functionaltests 47.50% <0.00%> (-0.07%) ⬇️
unittests 44.14% <0.00%> (ø)

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

Impacted Files Coverage Δ
cms/server/contest/handlers/taskusertest.py 46.55% <0.00%> (ø)
cms/server/admin/handlers/base.py 67.98% <0.00%> (-2.98%) ⬇️
cms/db/base.py 87.12% <0.00%> (-1.99%) ⬇️
cms/service/ScoringService.py 66.66% <0.00%> (-1.52%) ⬇️
cms/db/fsobject.py 76.51% <0.00%> (-1.52%) ⬇️
cms/service/esoperations.py 79.86% <0.00%> (-1.39%) ⬇️
cms/io/service.py 72.72% <0.00%> (-1.22%) ⬇️
cms/service/workerpool.py 65.55% <0.00%> (-1.12%) ⬇️
cms/io/rpc.py 93.19% <0.00%> (-1.03%) ⬇️
cms/server/contest/handlers/tasksubmission.py 50.27% <0.00%> (-0.55%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9236084...b920fe1. Read the comment docs.

@andreyv
Copy link
Member

andreyv commented Nov 10, 2020

Thanks for the fix :)

From the code, it looks like the datetime and time classes are intended for styling the first column. See cws_style.css and similar code in submission.html.

In particular, there seems to be another bug here: the col.time selector appears twice and overrides its previous definition.

What do you think about keeping datetime/time for the first column and re-classing the third column, in both colgroup and tbody, to something like execution_time? This would fix both problems and keep consistency with the submissions table.

@ranaldmiao
Copy link
Contributor Author

ranaldmiao commented Mar 7, 2021

What do you think about keeping datetime/time for the first column and re-classing the third column, in both colgroup and tbody, to something like execution_time? This would fix both problems and keep consistency with the submissions table.

It's been a while, I have made the changes as you suggested and rebased it on the current master branch.

Thanks for the review too! Would you mind reviewing it again?

@andreyv
Copy link
Member

andreyv commented Mar 10, 2021

Thanks! The fix looks good now. I adjusted the key in JSON response from time to execution_time as well.

@andreyv andreyv merged commit 2629aab into cms-dev:master Mar 10, 2021
@ranaldmiao ranaldmiao deleted the fix-1159 branch March 20, 2021 15:15
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.

2 participants