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

Migrate *Attributes time fields from Date to Instant #8636 #8637

Merged

Conversation

whipermr5
Copy link
Member

@whipermr5 whipermr5 commented Mar 14, 2018

Fixes #8636

Based on #8629. Commits start from 6768e02.

Script used to migrate timestamps in JSON data:

import json
import os, io
from collections import OrderedDict
from datetime import datetime, timedelta

P_FORMAT = '%Y-%m-%d %I:%M %p UTC'

filenames = [f for f in os.listdir('.') if os.path.isfile(f) and f.endswith('.json')]

for filename in filenames:
    print(filename)
    with open(filename) as file:
        data = json.loads(file.read(), object_pairs_hook=OrderedDict)

    try:
        if not data.get('feedbackResponseComments'):
            continue
    except:
        continue

    for fsKey, fs in data.get('feedbackResponseComments').iteritems():
        for key, val in fs.iteritems():
            if key in ['createdAt', 'lastEditedAt']:
                timestamp = datetime.strptime(fs[key], P_FORMAT)
                fs[key] = timestamp.isoformat() + 'Z'

    with io.open(filename, 'w', encoding='utf8') as file:
        file.write(json.dumps(data, indent=2, separators=(',', ': '), ensure_ascii=False) + '\n')

@whipermr5 whipermr5 added this to To do in Migration to java.time via automation Mar 14, 2018
@whipermr5 whipermr5 moved this from To do to In progress in Migration to java.time Mar 14, 2018
@whipermr5 whipermr5 added the s.ToReview The PR is waiting for review(s) label Mar 14, 2018
Copy link
Contributor

@tran-tien-dat tran-tien-dat left a comment

Choose a reason for hiding this comment

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

Very nice PR. Just a small comment.

@@ -259,26 +259,24 @@ public void testSanitizeForSaving() {

@Test
public void testSendDateForDisplay() {
Calendar calendar = formatDateForAdminEmailAttributesTest(validAdminEmailAttributesObject.sendDate);
String expectedDate = TimeHelper.formatTime12H(calendar.getTime());
validAdminEmailAttributesObject.sendDate = Instant.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change in the logic of the test intentional? The original test does not set this field. Ok, I see that this is for consistency with the test below.

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 original test coupled it together with formatDateForAdminEmailAttributesTest, which I believe was a mistake.

Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
return TimeHelper.convertToUserTimeZone(calendar, Const.SystemParams.ADMIN_TIME_ZONE_DOUBLE);
private LocalDateTime formatDateForAdminEmailAttributesTest(Instant date) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name doesn't make sense. It should be something like convertToAdminTime

Copy link
Contributor

@tran-tien-dat tran-tien-dat left a comment

Choose a reason for hiding this comment

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

LGTM

@whipermr5 whipermr5 force-pushed the 8636-migrate-attributes-time-fields-instant branch from 23170db to b82e8c7 Compare March 14, 2018 15:10
Copy link
Member

@darrenwee darrenwee left a comment

Choose a reason for hiding this comment

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

👍

@whipermr5 whipermr5 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 14, 2018
@whipermr5 whipermr5 requested a review from damithc March 14, 2018 15:16
@damithc damithc added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 14, 2018
@whipermr5 whipermr5 added this to the V6.4.0 milestone Mar 14, 2018
@whipermr5 whipermr5 added the e.1 label Mar 14, 2018
@whipermr5 whipermr5 merged commit dbca7a2 into TEAMMATES:master Mar 14, 2018
Migration to java.time automation moved this from In progress to Done Mar 14, 2018
@whipermr5 whipermr5 deleted the 8636-migrate-attributes-time-fields-instant branch March 14, 2018 15:45
@whipermr5 whipermr5 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants