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.

Sign up for GitHub

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

String PID support #511

Closed
wants to merge 13 commits into from
Closed

String PID support #511

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
862c1cb
String PID support
May 11, 2019
f0e4a9e
Fix templates
May 11, 2019
70f5475
Add a space
undefined-moe May 11, 2019
8434471
contest
undefined-moe May 11, 2019
46a5ad0
Remove lowercase PID support as it will cause an error for /contest/{…
undefined-moe May 11, 2019
e554066
Fix template
undefined-moe May 11, 2019
37137fd
Fix support for _id
undefined-moe May 11, 2019
57d5312
Allow users add String PID problems.
undefined-moe May 11, 2019
ac2af5b
Add validator
undefined-moe May 11, 2019
89aecbc
Change limit to 24
undefined-moe May 11, 2019
5b8e8a2
Update translation
undefined-moe May 12, 2019
9cefd5f
Add lowercase support
undefined-moe May 12, 2019
052ca0f
-
undefined-moe May 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vj4/handler/contest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ async def get(self, *, tid: objectid.ObjectId):
file_name='{}.zip'.format(tdoc['title']))


@app.route('/contest/{tid}/{pid:-?\d+|\w{24}}', 'contest_detail_problem')
@app.route('/contest/{tid}/{pid:[A-Z0-9]+}', 'contest_detail_problem')
class ContestDetailProblemHandler(contest.ContestMixin, base.Handler):
@base.route_argument
@base.require_perm(builtin.PERM_VIEW_CONTEST)
Expand Down
2 changes: 1 addition & 1 deletion vj4/handler/homework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ async def get(self, *, tid: objectid.ObjectId):
file_name='{}.zip'.format(tdoc['title']))


@app.route('/homework/{tid}/{pid:-?\d+|\w{24}}', 'homework_detail_problem')
@app.route('/homework/{tid}/{pid:[A-Z0-9]+}', 'homework_detail_problem')
Copy link
Member

Choose a reason for hiding this comment

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

Why not allowing lowercase letters and maybe other characters?

Copy link
Member Author

Choose a reason for hiding this comment

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

image
It creates conflict.

Copy link
Member

Choose a reason for hiding this comment

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

What conflict? Can you explain more?

Copy link
Member Author

Choose a reason for hiding this comment

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

not this route
my fault.
image
this one.

Copy link
Member

Choose a reason for hiding this comment

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

Emmm. Then I think we should not simply support string PIDs without changing the URL scheme. I think depending on URL case sensitivity is not a good idea.

By the way, it would be better to text instead of screenshot for code snippets. It is more friendly for more types of browsers/screen readers. For example:

@app.route(...)
class XxxHandler:
  pass

Copy link
Member Author

Choose a reason for hiding this comment

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

Some user may just collected a URL in his browser.
Changing the URL scheme will make then confused as it throws 404 error.

Copy link
Member

Choose a reason for hiding this comment

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

Of course we should change the URL in a backward compatible way, and preferably still generate the same URL for numeric/objectid PIDs. This can be achieved by making multiple routes to point to the same handler.

One proposal is to use (optional) named parameter in URL. For example: /homework/tid=xxx/pid=xxx/scoreboard. We can also use query strings.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Scoreboard for each problem?
so weird...

Copy link
Member

Choose a reason for hiding this comment

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

My bad. The example should be /homework/tid=xxx/pid=xxx/submit. It is just an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

the submit handler can work properly.
but handlers like 'homework_edit' 'homework_scoreboard' will be handled by 'homework_problem' handler if we enables lowercase pid.

class HomeworkDetailProblemHandler(contest.ContestMixin, base.Handler):
@base.route_argument
@base.require_perm(builtin.PERM_VIEW_HOMEWORK)
Expand Down
2 changes: 1 addition & 1 deletion vj4/handler/problem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ async def get(self, *, category: str):
self.json_or_redirect(self.referer_or_main)


@app.route('/p/{pid:-?\d+|\w{24}}', 'problem_detail')
@app.route('/p/{pid:[A-Z0-9]+}', 'problem_detail')
class ProblemDetailHandler(base.OperationHandler):
async def _get_related_trainings(self, pid):
if self.has_perm(builtin.PERM_VIEW_TRAINING):
Expand Down
2 changes: 1 addition & 1 deletion vj4/ui/templates/components/problem.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
{%- endif %}
>
{%- endif %}
{% if pdoc['doc_id']|string|length < 10 %}P{{ pdoc['doc_id'] }} {% endif %}{{ pdoc['title'] }}
{% if pdoc['doc_id'] is number %}P{{ pdoc['doc_id'] }} {% elif pdoc['doc_id']|string|length < 10 %} {{ pdoc['doc_id'] }} {% endif %}{{ pdoc['title'] }}
{%- if not invalid %}
</a>
{%- endif %}
Expand Down