Change timezone to only accept valid timezones #354

tankangliang · 2020-11-06T12:25:00Z

Description

Timezone class now references a set of valid timezones for validation. The format for parameters is now GMT+HH:MM and the list of valid timezones can be found here.

Fixes #282

Testing

As per UserGuide:

client add n/Katya p/98123456 e/katya@yahoo.com a/Vladivostok, Nevelskogo, bld. 15, appt. 256 c/RU tz/GMT+03:00 ce/22-12-2020
client edit 3 c/JP tz/GMT+07:00

Also manual testing with client view 1 and changing the timezones to verify they are of the correct timing.

codecov · 2020-11-06T12:26:50Z

Codecov Report

Merging #354 (4be1def) into master (422300b) will increase coverage by 0.08%.
The diff coverage is 95.65%.

@@             Coverage Diff              @@
##             master     #354      +/-   ##
============================================
+ Coverage     89.55%   89.64%   +0.08%     
+ Complexity      995      994       -1     
============================================
  Files           112      112              
  Lines          2586     2589       +3     
  Branches        304      303       -1     
============================================
+ Hits           2316     2321       +5     
  Misses          183      183              
+ Partials         87       85       -2

Impacted Files	Coverage Δ	Complexity Δ
...seedu/address/logic/commands/ClientAddCommand.java	`100.00% <ø> (ø)`	`8.00 <0.00> (ø)`
...edu/address/model/client/ClientSuggestionType.java	`66.66% <ø> (ø)`	`22.00 <0.00> (ø)`
.../java/seedu/address/model/util/SampleDataUtil.java	`95.45% <ø> (ø)`	`5.00 <0.00> (ø)`
...main/java/seedu/address/model/client/Timezone.java	`93.93% <95.23%> (+3.93%)`	`13.00 <7.00> (-1.00)`	⬆️
src/main/java/seedu/address/ui/WidgetViewBox.java	`95.34% <100.00%> (+1.16%)`	`18.00 <0.00> (ø)`

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 422300b...4be1def. Read the comment docs.

qwoprocks · 2020-11-06T15:00:55Z

docs/UserGuide.md

@@ -535,7 +535,7 @@ Parameter | Prefix | Constraints, Examples
 **EMAIL** | `e/` | Emails should be of the format local-part@domain. <br> e.g. `e/katya@yahoo.com`
 **ADDRESS** |`a/` | Addresses can take any values, and it should not be blank. <br> e.g. `Vladivostok, Nevelskogo, bld. 15, appt. 256`
 **COUNTRY_CODE** | `c/` | A 2-letter country code that follows the ISO3166 specification <br> This [finding tool](https://www.countrycode.org/) can be used. <br> e.g. `c/SG` (Singapore) 
-**TIMEZONE** | `tz/` | Timezone should be given in offsets relative to [Greenwich Mean Time](https://en.wikipedia.org/wiki/Greenwich_Mean_Time). <br> e.g. `tz/GMT+8`
+**TIMEZONE** | `tz/` | Timezone should be given in offsets relative to [Greenwich Mean Time](https://en.wikipedia.org/wiki/Greenwich_Mean_Time) in the format `GMT+HH:MM` where `HH` refers to the offset in hours and `MM` refers to the offset in minutes. The full list of valid timezones can be found [here](https://www.timeanddate.com/time/current-number-time-zones.html) <br> e.g. `tz/GMT+08:00


the link leads to a table with UTC timezones instead

can this be used instead? https://greenwichmeantime.com/time-gadgets/time-zone-converter/

qwoprocks · 2020-11-06T15:01:51Z

src/main/java/seedu/address/model/client/Timezone.java

-            + "-" + SMALLEST_NEGATIVE_OFFSET;
+    public static final Set<String> VALID_TIMEZONES = new HashSet<>();
+
+    static {


I think we should standardize (forgot which other file was it) and just remove the static, and add a comment above saying this is some special static init block

I only found the one in TbmManager.java but that one doesn't seem to be for class-level variables

Oh right ok

qwoprocks · 2020-11-06T15:03:31Z

src/main/java/seedu/address/model/client/Timezone.java

+        List<String> zoneList = new ArrayList<>(allZones);
+        for (String s : zoneList) {
+            ZoneOffset offset = LocalDateTime.now().atZone(ZoneId.of(s)).getOffset();
+            String out = String.format("%s", offset);
+            VALID_TIMEZONES.add(out);
+        }


Is there a need to convert to arraylist? Cannot just iterate through the set?

qwoprocks · 2020-11-06T15:03:46Z

src/main/java/seedu/address/model/client/Timezone.java

+        VALID_TIMEZONES.remove("Z");
+        VALID_TIMEZONES.add("+00:00");


Should probably add a comment stating what this does.

src/main/java/seedu/address/model/client/Timezone.java

qwoprocks · 2020-11-06T15:05:53Z

src/main/java/seedu/address/model/client/Timezone.java

+        if (zoneOffsetIdString.equals(GMT_STRING)) {
+            zoneOffsetIdString = GMT_STRING + "+00:00";
+        }


Is this some special case? Should add a comment if that's the case

qwoprocks · 2020-11-06T15:07:44Z

src/test/java/seedu/address/model/client/TimezoneTest.java

-            long totalHours = totalMinutes / 60;
-            long currentHour = ((totalHours + timezoneOffset) % 24);
+            long totalHours = (totalMinutes + minutesOffset) / 60;
+            long currentHour = ((totalHours + hoursOffset) % 24);


Suggested change

long currentHour = ((totalHours + hoursOffset) % 24);

long currentHour = (totalHours + hoursOffset) % 24;

qwoprocks · 2020-11-06T15:10:56Z

src/test/java/seedu/address/model/client/TimezoneTest.java

+        assertFalse(Timezone.isValidTimezone("GM+00:00")); // misspelled GMT
+        assertFalse(Timezone.isValidTimezone("GMT+15:00")); // out of range (positive)
+        assertFalse(Timezone.isValidTimezone("GMT-13:00")); // out of range (negative)
+        assertFalse(Timezone.isValidTimezone("GMT+13:30")); // unrecognised timezone
        assertFalse(Timezone.isValidTimezone("GMT+654657987654456")); // large number


Should add a few more negative test cases like "GMT" and "GMT+" and "GMT-", and maybe "gmt+08:00", and "GMT+8" and "GMT+08", and "GMT+8:00", and "GMT+08:0"

qwoprocks · 2020-11-06T15:11:23Z

src/test/java/seedu/address/logic/commands/ClientAddCommandIntegrationTest.java

@@ -29,6 +29,7 @@ public void setUp() {
    public void execute_newClient_success() {
        Client validClient = new ClientBuilder().build();

+


extra newline

…into timezone

qwoprocks

Small comments, but LGTM

qwoprocks · 2020-11-07T17:44:32Z

src/main/java/seedu/address/model/client/Timezone.java

+        VALID_TIMEZONES.remove("Z");
+        VALID_TIMEZONES.add("+00:00");


qwoprocks · 2020-11-07T17:45:54Z

src/main/java/seedu/address/model/client/Timezone.java

@@ -41,11 +53,9 @@
    public Timezone(String timezone) {
        requireNonNull(timezone);
        checkArgument(isValidTimezone(timezone), MESSAGE_CONSTRAINTS);
-        String offsetString = timezone.substring(GMT_STRING.length());
-        int offsetValue = Integer.parseInt(offsetString);
+        assert (VALID_TIMEZONES.contains(timezone.substring(UTC_STRING.length())));


Suggested change

assert (VALID_TIMEZONES.contains(timezone.substring(UTC_STRING.length())));

assert VALID_TIMEZONES.contains(timezone.substring(UTC_STRING.length()));

btw what does this assert do? Isit rly needed lmao

tankangliang added 3 commits November 6, 2020 20:14

Change timezone to only accept valid timezones

90ad4dd

Fix checkstyle issues

ff8f5e9

Fix old timezone command in user guide

8e0f7f4

tankangliang added the priority.High Must do label Nov 6, 2020

tankangliang added this to the v1.4 milestone Nov 6, 2020

tankangliang self-assigned this Nov 6, 2020

tankangliang added this to In progress in v1.4 via automation Nov 6, 2020

tankangliang requested review from raysonkoh and qwoprocks November 6, 2020 12:25

Merge branch 'master' into timezone

41350c5

qwoprocks requested changes Nov 6, 2020

View reviewed changes

v1.4 automation moved this from In progress to Review in progress Nov 6, 2020

tankangliang added 3 commits November 7, 2020 17:09

Merge branch 'master' into timezone

b99e07c

Change GMT to UTC

69f7980

Merge branch 'master' of https://github.com/AY2021S1-CS2103T-F11-4/tp …

a42db47

…into timezone

tankangliang requested review from qwoprocks and rtshkmr November 7, 2020 16:47

v1.4 automation moved this from Review in progress to Reviewer approved Nov 7, 2020

qwoprocks approved these changes Nov 7, 2020

View reviewed changes

Fix PR issues

4be1def

tankangliang merged commit 08c1232 into AY2021S1-CS2103T-F11-4:master Nov 7, 2020

v1.4 automation moved this from Reviewer approved to Done Nov 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change timezone to only accept valid timezones #354

Change timezone to only accept valid timezones #354

tankangliang commented Nov 6, 2020

codecov bot commented Nov 6, 2020 •

edited

qwoprocks Nov 6, 2020

rtshkmr Nov 7, 2020

qwoprocks Nov 6, 2020

tankangliang Nov 7, 2020

qwoprocks Nov 7, 2020

qwoprocks Nov 6, 2020

qwoprocks Nov 6, 2020

qwoprocks Nov 7, 2020

qwoprocks Nov 6, 2020

qwoprocks Nov 6, 2020

qwoprocks Nov 6, 2020

qwoprocks Nov 6, 2020

qwoprocks left a comment

qwoprocks Nov 7, 2020

qwoprocks Nov 7, 2020

	long currentHour = ((totalHours + hoursOffset) % 24);
	long currentHour = (totalHours + hoursOffset) % 24;

		@@ -29,6 +29,7 @@ public void setUp() {
		public void execute_newClient_success() {
		Client validClient = new ClientBuilder().build();

	assert (VALID_TIMEZONES.contains(timezone.substring(UTC_STRING.length())));
	assert VALID_TIMEZONES.contains(timezone.substring(UTC_STRING.length()));

Change timezone to only accept valid timezones #354

Change timezone to only accept valid timezones #354

Conversation

tankangliang commented Nov 6, 2020

Description

Testing

codecov bot commented Nov 6, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwoprocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2020 •

edited