-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Allow custom formatting of due date strings. #916
Conversation
@markchang This is ready for testing. Before you check out the branch, set show_timezone=False in a Studio course. That way you can test the migration path after you check out the branch (show_timezone will not be visible in Advanced Settings). |
@sarina Can you code review? This touches LMS files. @singingwolfboy can I get a review from you as well? |
Added some context in the description. |
Could some documentation be added giving examples of legal |
We don't have documentation in Studio for things under "Advanced Settings". The expectation is that how to use these properties will be communicated to course staff on a need-to-know basis. @markchang has purposefully chosen to have us NOT display the help strings for these items. We really, really hope that very few people will feel the need to change how due dates are formatted.... If there is static documentation someplace for this sort of thing, I'm happy to update that. |
Ah - well, every on-campus course at MIT using the edX-platform will want to change the date string, based on feedback we're getting. Folks want (1) dates in EST / EDT, and (2) simplified dates, with less confusing detail. I would guess that any residential campus use of the platform will want similarly. |
Note that the dates are still really UTC dates-- we only store UTC dates on the server. If someone displays them as any other timezone, they have to modify the graceperiod accordingly to "fake it". If this really is a feature that will be widely used, it should be promoted out of Advanced Settings into the actual course settings (it would make sense to put it along side the grace period). I'll put a story on the backlog for that and we can monitor feedback. |
We also would like at some point in the future to have display of dates depend on the client browser (so dates would be translated properly into the user's timezone). This would address #1 and would require passing the formatting string to the client. |
That's reasonable. Thanks, @cahrens |
timezone = dt.strftime('%z') | ||
else: | ||
timezone = u" UTC" | ||
return unicode(dt.strftime(u"%b %d, %Y at %H:%M{tz}")).format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should add a comment here explaining why we can't internationalize the word "at"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the strings in common aren't translated (and there's a lot of them). I don't think we want to put comments throughout the code.
(BTW, I already bullied DB in person about this-- just updating the PR to reflect that).
Looks good to me. 👍 |
@@ -20,7 +20,8 @@ class CourseMetadata(object): | |||
'enrollment_end', | |||
'tabs', | |||
'graceperiod', | |||
'checklists'] | |||
'checklists', | |||
'show_timezone'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
]
on final line, as per https://github.com/edx/edx-platform/wiki/Python-Guidelines ("Breaking long lines" section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it applies in this case. It is not showing up in the pep8/pylint violations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a violation, just a style we're aiming for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that guideline is specifically about function calls. Unless I'm missing something....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh I always thought it was for all long things, no big deal then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked around LMS team. It's a preferred style thing - we all do the indentation of long lines the same way for function calls as well as long structs like lists, tuples, dictionaries. but it's not a requirement, just a preference for consistent styling in our code base. so whatever I suppose you want to do as long as that's clear.
Local dev environment finally working! All seems good to me. |
OK, addressed pep8/pylint warnings and also changed the style of list. Thanks for the review, @sarina. So you are 👍 after the tests have run again? |
Yep 👍 😎 On Thu, Sep 12, 2013 at 1:29 PM, Christina Roberts <notifications@github.com
|
STUD-724 Test course for allowing custom formatting of due date strings. STUD-724 Try making the name of the course match the folder name. More cleanup. More cleanup. updates.
…ow_timezone=False to using due_date_display_format.
@ichuang all good points. we are putting this in as a quick stop gap to give ultimate control for those that really want it (and it is not super easy, as you pointed out). Otherwise, it is fairly low priority (given other work) to surface a comfortable UI for tweaking this display element. I hope that clarifies where we are coming from. |
Allow custom formatting of due date strings.
* addition of test code of the certificate issuance function openedx#2038 (openedx#2054) * Mod translation of 'Course End Date:' openedx#2084 (openedx#2090) * Add menu to ga_operation for ga_analyzer openedx#2039 (openedx#2088) * Fixed bugs openedx#2039 (openedx#2112) * Fixed csv format openedx#2039 (openedx#2127) * Change to split download if there are many display items openedx#916 (openedx#2121) * Change to split download if there are many display items openedx#916 * Fix UT * Fix Review * Fix review2
* Add menu to ga_operation for ga_analyzer openedx#2039 (openedx#2088) * add role for old course viewer openedx#2062 (openedx#2087) * add role for old course viewer openedx#2062 * Change action for biz course by BetaTester role openedx#2062 * Construction of image server openedx#2025 (openedx#2106) * cherry-pick 8c8953f * Fix file upload in IE * Construction of image server openedx#2025 * add all keywords search in Student management openedx#2029 (openedx#2034) * Fix bug for before enrollment start in ga old course viewer openedx#2062 (openedx#2125) * fix. Construction of image server openedx#2025 (openedx#2117) * Modify message and css of enrollment for Course About openedx#2130 * Add a certificate list to user's profile page. openedx#2042 (openedx#2108) * Mod UT openedx#2130 * add PDF File Construction of image server openedx#2025 (openedx#2140) * add library option, and library links to the course. openedx#2001 (openedx#2124) * Invalid StudioPermissionsService object in API to show/save xblock settings in CMS. Randomized Content Block editor did not check Studio user's permissions * add library option, and library links to the course. openedx#2001 * fix. add all keywords search in Student management openedx#2029 (openedx#2034) (openedx#2157) * second fix. Construction of image server openedx#2025 (openedx#2158) * add library option, and library links to the course. openedx#2001 (openedx#2160) * third fix. Construction of image server openedx#2163 (openedx#2164) * Add filter by category for certificates on profile page openedx#2042 (openedx#2165) * Fix bug for add library option, and library links to the course. openedx#2162 openedx#2133 (openedx#2167) * Develop/dogwood/gacco201708 (openedx#2170) * Fixed bugs openedx#2039 (openedx#2112) * Fixed csv format openedx#2039 (openedx#2127) * Change to split download if there are many display items openedx#916 (openedx#2121) * Change to split download if there are many display items openedx#916 * Fix UT * Fix Review * Fix review2
YONK-768: Discussions score issue fixed
Fix CGS translation typo
* Commits: Remove call: to_deprecated_string Split out nonregistered aka sneakpeek
STUD-724
Allow specification of a format for due dates in LMS. See http://docs.python.org/2/library/datetime.html#strftime-strptime-behavior for syntax of format string.
For context, the issue here is that last fall we started showing the timezone on due dates. This bothered some course authors because they were using grace periods to "mask" what the timezone is. In response to their feedbac, we added a setting to hide the timezone ("show_timezone"). However, come this semester, it turns out that what they REALLY want is to hide the time part of the date completely. I decided to solve this by allowing them to completely control the formatting of the due date (which is how I wish I had done it in the past....).
Set the due date format in Studio, Advanced Settings, under the key "due_date_display_format".
Includes unit tests for the 2 places in LMS code where the due date is displayed (left-hand accordion, progress page).
Also removed a usage of django.utils.translation in common, as common should not depend on django. Currently we have no i18n process for things in common.