Customize Labels Step 1 #1110

jiji14 · 2023-12-01T20:29:34Z

Related Issue

System Design: GitHub Issue 1012
Server: GitHub Server Pull Request 946

What I added

Divided default label and custom label.
Saved user's custom mode.

Next Step

Manage "My Labels" section in the Profile tab.
Delete label function.

Result

Please check out to `customize-modes` branch in the `Server` repository for testing.

codecov · 2023-12-04T19:00:43Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (9ddcf31) 77.55% compared to head (4e484a9) 76.96%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
- Coverage   77.55%   76.96%   -0.59%     
==========================================
  Files          28       28              
  Lines        1702     1715      +13     
  Branches      367      367              
==========================================
  Hits         1320     1320              
- Misses        382      395      +13

Flag	Coverage Δ
unit	`76.96% <0.00%> (-0.59%)`	⬇️

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

Files	Coverage Δ
www/js/services/commHelper.ts	`22.55% <0.00%> (-2.45%)`	⬇️

increase new mode frequency and decrease old mode frequency

…setup

jiji14 · 2023-12-16T00:49:08Z

@JGreenlee
I wanted to add unit tests for getModes and updateMode functions but it seems like there's no point in just mockingBEMServerComm

JGreenlee

Good first steps. Interface looks great!
Left some comments for code cleanliness + an open question about performance

www/js/survey/multilabel/MultiLabelButtonGroup.tsx

www/__tests__/commHelper.test.ts

www/js/services/commHelper.ts

JGreenlee · 2023-12-17T07:52:03Z

www/js/survey/multilabel/MultiLabelButtonGroup.tsx

  const chosenLabel = useMemo<string>(() => {
    if (otherLabel != null) return 'other';
    return timelineLabelMap[trip._id.$oid]?.[modalVisibleFor]?.value;
  }, [modalVisibleFor, otherLabel]);
-
+  const initialLabel = chosenLabel;


I don't see the purpose of this variable. It seems like initialLabel will always be the same as chosenLabel.

It is never reassigned (and can't be anyway because it is const)

The reason I used initialLabel was that I couldn't access the 'chosenLabel' value because the store function had a 'chosenLabel' parameter.

To access chosenLabel, I needed to either change the parameter name from chosenLabel to a different one or create a new variable that contains chosenLabel. That's why I declared 'initialLabel.'

I deleted line 50, changed chosenLabel to initialLabel, and modified the parameter name from chosenLabel to newLabel. I think it seems more intuitive.

Let me know if it doesn't make sense to you

JGreenlee

Great step forward! The logic in MultiLabelButtonGroup is cleaner now and I think this will be better for network performance.

Now, I have some questions about the design: Is this supposed to work for custom purposes too?

If so, it needs to be made more generic, and the feature may be more accurately described as "custom labels" rather than just "custom modes".

If not, we need to fix regressions. The current behavior is that the same list of "Custom modes" shows up for both "mode" and "purpose" popups, which is not what we want.

JGreenlee · 2024-01-03T19:10:59Z

Here is what I mean by the same list of custom modes showing up in both popups

I think these should be kept as two separate lists, and the text should reflect that

TODO : handling custom purpose label

jiji14 · 2024-01-03T20:21:52Z

Here is what I mean by the same list of custom modes showing up in both popups

I think these should be kept as two separate lists, and the text should reflect that

Thanks for reporting this! I forgot to add to check inputType === 'MODE'. I will submit a different PR for PURPOSE later.

JGreenlee

If the expectation for this PR is to only support custom modes, and not custom purposes / custom replaced modes, it looks ready.

However, if the expectation is to have both custom modes and custom purposes supported (as was supported previously), I would still suggest revising this PR to a more generic "custom labels" implementation. It would be more efficient to do in one PR rather than two.

Sending this along to the next step of review so @shankari can weigh in.

…replaced_mode ...) functions to em

jiji14 · 2024-01-05T20:43:39Z

@JGreenlee
After rethinking the design, I agreed that it would be more readable and efficient to modify functions to accommodate all types of labels (mode, purpose, and replaced_mode) and dynamically change them with a key. I have also updated the server code, so please pull both branches if you want to run this. Thanks for your advice!

JGreenlee

I have a few other nitpicks for code style and tidieness left below

But when I tried to test this (I was using this branch customize-modes and the server branch customize-modes), I got the following peculiar result:

When I selected "Other" to add a custom mode, the text box appeared below, but the radio button next to "Other" did not fill in, making it look like "Other" was not selected

If I continued with trying to add a custom mode and clicked "Save", I got this message:

The custom label does show up on that trip:

But not in the 'custom modes' list:

JGreenlee · 2024-01-09T18:17:24Z

www/js/diary/LabelTab.tsx

 } from './LabelTabContext';

 let showPlaces;
 const ONE_DAY = 24 * 60 * 60; // seconds
 const ONE_WEEK = ONE_DAY * 7; // seconds
+const CUSTOM_LABEL_KEYS = ['mode', 'purpose', 'replaced_mode'];


Why lowercase? As you may have noticed, the rest of the codebase uses uppercase.
I think if you stuck to uppercase, you wouldn't have to use .toLowercase() a bunch of times

I opted for lowercase because the keys in the database are stored in lowercase. This way, I aimed to send the key to the server without the need for any preprocessing. I changed variable name to CUSTOM_LABEL_KEYS_IN_DATABASE. What do you think about it?

That's a good reason and makes sense. Thanks for clarifying!

JGreenlee · 2024-01-09T18:20:37Z

www/js/survey/multilabel/MultiLabelButtonGroup.tsx

+          customLabel[key] = res['label'];
+          setCustomLabel(customLabel);


I would use ... here to create a shallow copy of the object with the new key added. This is to avoid mutating the old customLabel object

Suggested change

customLabel[key] = res['label'];

setCustomLabel(customLabel);

setCustomLabel({

...customLabel,

[key]: rest['label'],

});

JGreenlee · 2024-01-09T18:25:39Z

www/js/diary/LabelTabContext.ts

+export type CustomLabelKey = {
+  [k: string]: string[];
+};


When I initially saw customLabel I thought it must contain just one label. Now that I am looking at the type definition I understand that it is an object of keys to string arrays.

Would you mind renaming customLabel to something like customLabelMap, or at least customLabels ?

Sorry to be picky but I think this could help others in the future comprehend the code better

jiji14 · 2024-01-09T22:06:52Z

@JGreenlee
Thank you for assisting me in writing cleaner code! Your comments are very helpful. I updated some variable names and fixed the issue with the 'other' radio button. While attempting to replicate the issue you mentioned, I encountered no errors. I tested with a new account and also devised a test case for scenarios where the label keys (mode, purpose, replaced_mode) are not present in the database. Considering the server code got changed, the error might have been due to outdated server code. Could you please check if the server code is up-to-date? If not, kindly pull the latest changes and retry. Thank you! 🙏

JGreenlee

Great! I created a new test user and verified that the error did not occur, so that was my mistake.
I also verified that the other comments have been addressed.

All I have left are a couple design questions:

Should the same list of custom modes be shared for "mode" and "replaced mode" ?
-- It was clear before that "mode" and "purpose" should be kept as separate lists. But after testing this branch and envisioning day-to-day use, I think it may be more logical for "replaced mode" to use the same list of custom labels as "mode".
Is it necessary to have the section title "Default Mode", "Default Purpose", etc?
-- I think this takes up extra space and the layout would be clearer without it.

jiji14 · 2024-01-11T22:01:08Z

That's a very good point! I was also unsure in separating 'mode' and 'replaced_mode'. I agree with using the same list. Please check my updates!

JGreenlee

LGTM!

…-mission-phone into customize-modes

JGreenlee · 2024-02-06T18:11:31Z

Resolved merge conflicts.

@jiji14 Can you please change the target of this PR to master?

jiji14 · 2024-02-06T18:45:03Z

Resolved merge conflicts.

@jiji14 Can you please change the target of this PR to master?

Done!

shankari · 2024-02-12T20:51:22Z

@jiji14 I am reviewing this, but please note that the codecov tests are failing. The run-codecov task is passing, but the results of running the code coverage is that you don't have enough tests (the diff is not covered and the code coverage will go down thanks to this PR). Can you please add the appropriate tests before I merge?

jiji14 · 2024-02-12T21:26:59Z

@jiji14 I am reviewing this, but please note that the codecov tests are failing. The run-codecov task is passing, but the results of running the code coverage is that you don't have enough tests (the diff is not covered and the code coverage will go down thanks to this PR). Can you please add the appropriate tests before I merge?

@shankari We're currently facing challenges with testing API fetching functions, as they rely on native functions within BEMServerComm. While considering mocking these native functions for testing purposes, we initially deemed it unnecessary, as it would involve skipping tests for the actual API fetching functions. However, I will do some research about it!

Here are the comments about the test:

/* The following functions from commHelper.ts are not tested because they are just wrappers
    around the native functions in BEMServerComm.
  If we wanted to test them, we would need to mock the native functions in BEMServerComm.
    It would be better to do integration tests that actually call the native functions.

  * - getRawEntries
  * - getRawEntriesForLocalDate
  * - getPipelineRangeTs
  * - getPipelineCompleteTs
  * - getMetrics
  * - getAggregateData
  * - registerUser
  * - updateUser
  * - getUser
  * - putOne
  * - getModes
  * - updateMode
*/

shankari · 2024-02-13T02:25:00Z

@jiji14 I saw that, but codecov didn't 😄 It is possible to mark these as integration wrapper functions so that codecov only considers them for integration tests?

JGreenlee · 2024-02-13T20:44:09Z

I am going to suggest that we close this PR and focus on the Step 2 PR (#1120), because:

The Step 2 PR already includes all of these Step 1 changes, plus more
There are bugs with getting Step 1 and Step 2 to work together, which should be resolved before we roll out Custom Labels
If @jiji14 adds tests / excludes files from coverage here, she will have to also pull that into Step 2, and it's simpler to just do it in one PR

So concretely, my suggestions for @jiji14:

Fix the bugs in Custom Labels Step 2 #1120
Then exclude commHelper from the Jest coverage report (https://stackoverflow.com/a/55415228)
Make sure Custom Labels Step 2 #1120 works, mark it for review and make sure it passes all checks

jiji14 · 2024-02-14T00:55:11Z

I am going to suggest that we close this PR and focus on the Step 2 PR (#1120), because:

The Step 2 PR already includes all of these Step 1 changes, plus more

There are bugs with getting Step 1 and Step 2 to work together, which should be resolved before we roll out Custom Labels

If @jiji14 adds tests / excludes files from coverage here, she will have to also pull that into Step 2, and it's simpler to just do it in one PR

So concretely, my suggestions for @jiji14:

Fix the bugs in Custom Labels Step 2 #1120

Then exclude commHelper from the Jest coverage report (https://stackoverflow.com/a/55415228)

Make sure Custom Labels Step 2 #1120 works, mark it for review and make sure it passes all checks

@JGreenlee
Good idea! Thanks for your suggestions :)

show customized modes (used dummy data for now)

1b57f5e

jiji14 marked this pull request as draft December 1, 2023 20:29

jiji14 added 3 commits December 1, 2023 13:22

fix prettier issu

cad2d7c

fetch post/mode

74e2dfe

use displayErrorMsg

0402a34

jiji14 added 7 commits December 6, 2023 19:13

Done with fetching data to server

09b56ee

update mode - old mode and new mode

a3cb1f2

increase new mode frequency and decrease old mode frequency

Change modes type to string array and call it when modal opens

abc7f8a

add comments

cb1c491

change mode text in a desired format (ex) my_car => My Car

0a7a4ba

add default mode & custom mode label

8ba3fb6

noted that getModes & updateMode need to be tested after e2e testing …

aa18e98

…setup

jiji14 mentioned this pull request Dec 16, 2023

User Customize Labels e-mission/e-mission-server#946

Merged

jiji14 marked this pull request as ready for review December 16, 2023 00:42

JGreenlee requested changes Dec 17, 2023

View reviewed changes

jiji14 added 7 commits December 18, 2023 11:59

reuse existing common function and delete the original function

7548de7

use dynamic color

01c1afe

change logic for showing custom modes before 'other' option

b5a63be

change function names more descriptive

f64d719

delete unnecessary variable and change variable names more intuitive

14c4438

add new translation entry

447e7e2

Fetch custom modes on initial rendering to prevent unnecessary api calls

0509d5e

JGreenlee requested changes Jan 3, 2024

View reviewed changes

check if inputType is 'MODE' for custom mode label

189f79d

TODO : handling custom purpose label

JGreenlee reviewed Jan 4, 2024

View reviewed changes

jiji14 marked this pull request as draft January 4, 2024 19:45

Modify the mode functions to encompass all types of labels (purpose, …

643ae90

…replaced_mode ...) functions to em

jiji14 changed the title ~~Customize modes Step 1~~ Customize Labels Step 1 Jan 5, 2024

jiji14 marked this pull request as ready for review January 5, 2024 20:44

JGreenlee requested changes Jan 9, 2024

View reviewed changes

jiji14 added 2 commits January 9, 2024 13:39

fix radio button issue when 'other' option is selected

01a3729

change variable names for better understanding

8cec495

update radio button comment

606816b

JGreenlee reviewed Jan 11, 2024

View reviewed changes

combine replaced_mode and mode

2d7352b

JGreenlee approved these changes Jan 12, 2024

View reviewed changes

jiji14 mentioned this pull request Jan 12, 2024

Custom Labels Step 2 #1120

Merged

Merge branch 'service_rewrite_2023' of https://github.com/e-mission/e…

4e484a9

…-mission-phone into customize-modes

jiji14 changed the base branch from service_rewrite_2023 to master February 6, 2024 18:44

jiji14 closed this Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customize Labels Step 1 #1110

Customize Labels Step 1 #1110

jiji14 commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading

jiji14 commented Dec 16, 2023

JGreenlee left a comment

JGreenlee Dec 17, 2023

jiji14 Dec 18, 2023 •

edited

Loading

JGreenlee left a comment

JGreenlee commented Jan 3, 2024 •

edited

Loading

jiji14 commented Jan 3, 2024

JGreenlee left a comment

jiji14 commented Jan 5, 2024

JGreenlee left a comment •

edited

Loading

JGreenlee Jan 9, 2024

jiji14 Jan 9, 2024

JGreenlee Jan 10, 2024

JGreenlee Jan 9, 2024

JGreenlee Jan 9, 2024

jiji14 commented Jan 9, 2024

JGreenlee left a comment

jiji14 commented Jan 11, 2024

JGreenlee left a comment

JGreenlee commented Feb 6, 2024

jiji14 commented Feb 6, 2024

shankari commented Feb 12, 2024

jiji14 commented Feb 12, 2024

shankari commented Feb 13, 2024 •

edited

Loading

JGreenlee commented Feb 13, 2024

jiji14 commented Feb 14, 2024

		customLabel[key] = res['label'];
		setCustomLabel(customLabel);

-          customLabel[key] = res['label'];
-          setCustomLabel(customLabel);
+          setCustomLabel({
+            ...customLabel,
+            [key]: rest['label'],
+          });

Customize Labels Step 1 #1110

Customize Labels Step 1 #1110

Conversation

jiji14 commented Dec 1, 2023 • edited Loading

Related Issue

What I added

Next Step

Result

Please check out to customize-modes branch in the Server repository for testing.

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

jiji14 commented Dec 16, 2023

JGreenlee left a comment

Choose a reason for hiding this comment

JGreenlee Dec 17, 2023

Choose a reason for hiding this comment

jiji14 Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

JGreenlee left a comment

Choose a reason for hiding this comment

JGreenlee commented Jan 3, 2024 • edited Loading

jiji14 commented Jan 3, 2024

JGreenlee left a comment

Choose a reason for hiding this comment

jiji14 commented Jan 5, 2024

JGreenlee left a comment • edited Loading

Choose a reason for hiding this comment

JGreenlee Jan 9, 2024

Choose a reason for hiding this comment

jiji14 Jan 9, 2024

Choose a reason for hiding this comment

JGreenlee Jan 10, 2024

Choose a reason for hiding this comment

JGreenlee Jan 9, 2024

Choose a reason for hiding this comment

JGreenlee Jan 9, 2024

Choose a reason for hiding this comment

jiji14 commented Jan 9, 2024

JGreenlee left a comment

Choose a reason for hiding this comment

jiji14 commented Jan 11, 2024

JGreenlee left a comment

Choose a reason for hiding this comment

JGreenlee commented Feb 6, 2024

jiji14 commented Feb 6, 2024

shankari commented Feb 12, 2024

jiji14 commented Feb 12, 2024

shankari commented Feb 13, 2024 • edited Loading

JGreenlee commented Feb 13, 2024

jiji14 commented Feb 14, 2024

jiji14 commented Dec 1, 2023 •

edited

Loading

Please check out to `customize-modes` branch in the `Server` repository for testing.

codecov bot commented Dec 4, 2023 •

edited

Loading

jiji14 Dec 18, 2023 •

edited

Loading

JGreenlee commented Jan 3, 2024 •

edited

Loading

JGreenlee left a comment •

edited

Loading

shankari commented Feb 13, 2024 •

edited

Loading