feat: [AXIMST-93, 99, 105, 518, 537] Group configuration - Experiment Groups #188

ruzniaievdm · 2024-02-26T17:46:53Z

Add-Experiment-Groups
Delete-Experiment-Group
Edit-Experiment-Group
No-Internet-connection-alert-is-shown-with-incorrect-styles
sidebar-text-isnt-aligned-with-legacy

The main group configuration page

There were added actions to handle group configuration:

Create

Edit

Delete

PKulkoRaccoonGang

@ruzniaievdm The feature with percentage looks pretty cool!!!
I have a couple of suggestions:

Should the font size of error text be the same?

In the current implementation, the user can create a group with an empty value (if you add spaces instead of text, the groups can be saved)

src/group-configurations/GroupConfigurations.scss

src/group-configurations/common/UsageList.jsx

src/group-configurations/data/api.js

src/group-configurations/enrollment-track-groups-section/index.jsx

src/group-configurations/experiment-configurations-section/ExperimentForm.jsx

src/group-configurations/experiment-configurations-section/ExperimentFormGroups.jsx

src/group-configurations/experiment-configurations-section/utils.js

monteri · 2024-02-28T13:02:18Z

src/group-configurations/content-groups-section/messages.js

+  },
+  subtitleModalDelete: {
+    id: 'course-authoring.group-configurations.container.delete-modal.subtitle',
+    defaultMessage: 'content group',


[question] should be lowercase?

Yes we should it used like changing variable for sentence

monteri · 2024-02-28T13:03:19Z

src/group-configurations/data/api.js

+}
+
+/**
+ * Delete exists experimental configuration from the course.


Suggested change

* Delete exists experimental configuration from the course.

* Delete existing experimental configuration from the course.

monteri · 2024-02-28T13:05:15Z

src/group-configurations/data/api.js

+ * @param {object} configuration
+ * @returns {Promise<Object>}
+ */
+export async function createExperimentConfiguration(courseId, configuration) {


[question] Do we delete configuration or group? In case if we delete a group it requires renaming to createExperimentGroup. For other functions the same question.

We remove exactly configuration.
There are types content group, experiment configuration with actions.
That's what I called such them

monteri · 2024-02-28T13:13:36Z

src/group-configurations/experiment-configurations-section/ExperimentCard.test.jsx

+      configuration: experimentConfigurationUpdated,
+    });
+
+    const cardTitle = getByTestId('configuration-card-header__button');


[nit] I think this double underscore here is not needed. Let's refactor a bit.

Suggested change

const cardTitle = getByTestId('configuration-card-header__button');

const cardTitle = getByTestId('configuration-card-header-button');

monteri · 2024-02-28T13:15:04Z

src/group-configurations/experiment-configurations-section/ExperimentCardGroup.jsx

+      {groups.map((item) => (
+        <div
+          className="configuration-card-content__experiment-stack"
+          data-testid="configuration-card-content__experiment-stack"


Suggested change

data-testid="configuration-card-content__experiment-stack"

data-testid="configuration-card-content-experiment-stack"

monteri · 2024-02-28T13:18:02Z

src/group-configurations/experiment-configurations-section/ExperimentForm.jsx

+          values, errors, dirty, handleChange, handleSubmit,
+        }) => (
+          <>
+            <Form.Group className="mt-3 form-group-configuration" isInvalid={!!errors.name}>


Suggested change

<Form.Group className="mt-3 form-group-configuration" isInvalid={!!errors.name}>

<Form.Group className="mt-3 configuration-form-group" isInvalid={!!errors.name}>

To stick to the pattern above.

monteri · 2024-02-28T13:21:57Z

src/group-configurations/experiment-configurations-section/ExperimentFormGroups.jsx

+
+          return (
+            <Form.Group
+              key={idx}


Suggested change

key={idx}

key={group.name}

will it be unique or maybe group.id? If you change here you can remove eslint-disable on top of the file

export const initialExperimentConfiguration = { name: '', description: '', groups: [ { name: 'Group A', version: 1, usage: [] }, { name: 'Group B', version: 1, usage: [] }, ], scheme: 'random', parameters: {}, usage: [], active: true, version: 1, };

This component handle each group item, so we don't always have id field

Because it is initial groups without preinstalled id value

monteri · 2024-02-28T13:27:41Z

src/group-configurations/experiment-configurations-section/utils.js

+const getNextGroupName = (groups, groupFieldName = 'name') => {
+  const existingGroupNames = groups.map((group) => group.name);
+
+  let nextIndex = existingGroupNames.length + 1;
+
+  let groupName = '';
+  while (nextIndex > 0) {
+    groupName = ALPHABET_LETTERS[(nextIndex - 1) % 26] + groupName;
+    nextIndex = Math.floor((nextIndex - 1) / 26);
+  }
+
+  let counter = 0;
+  let newName = groupName;
+  while (existingGroupNames.includes(`Group ${newName}`)) {
+    counter++;
+    let newIndex = existingGroupNames.length + 1 + counter;
+    groupName = '';
+    while (newIndex > 0) {
+      groupName = ALPHABET_LETTERS[(newIndex - 1) % 26] + groupName;
+      newIndex = Math.floor((newIndex - 1) / 26);
+    }
+    newName = groupName;
+  }
+  return { [groupFieldName]: `Group ${newName}`, version: 1, usage: [] };
+};


Suggested change

const getNextGroupName = (groups, groupFieldName = 'name') => {

const existingGroupNames = groups.map((group) => group.name);

let nextIndex = existingGroupNames.length + 1;

let groupName = '';

while (nextIndex > 0) {

groupName = ALPHABET_LETTERS[(nextIndex - 1) % 26] + groupName;

nextIndex = Math.floor((nextIndex - 1) / 26);

}

let counter = 0;

let newName = groupName;

while (existingGroupNames.includes(`Group ${newName}`)) {

counter++;

let newIndex = existingGroupNames.length + 1 + counter;

groupName = '';

while (newIndex > 0) {

groupName = ALPHABET_LETTERS[(newIndex - 1) % 26] + groupName;

newIndex = Math.floor((newIndex - 1) / 26);

}

newName = groupName;

}

return { [groupFieldName]: `Group ${newName}`, version: 1, usage: [] };

};

const getNextGroupName = (groups, groupFieldName = 'name') => {

const existingGroupNames = new Set(groups.map((group) => group[groupFieldName]));

let baseName = 'A';

let counter = 1;

while (existingGroupNames.has(`Group ${baseName}${counter}`)) {

counter++;

if (counter > 26) {

baseName = String.fromCharCode(baseName.charCodeAt(0) + 1); // Increment base letter

counter = 1;

}

}

return { [groupFieldName]: `Group ${baseName}${counter}`, version: 1, usage: [] };

};

Add variables where you think is needed

This is not what we need to have/
Your solution generates groups in the sequence [A1, A2, A3,..]

monteri · 2024-02-28T13:29:36Z

src/group-configurations/experiment-configurations-section/utils.js

+ * @param {Array} groups - An array of group objects.
+ * @returns {boolean} True if all group names are unique, otherwise false.
+ */
+const allGroupNameAreUnique = (groups) => {


Suggested change

const allGroupNameAreUnique = (groups) => {

const allGroupNamesAreUnique = (groups) => {

monteri · 2024-02-28T13:30:35Z

src/group-configurations/experiment-configurations-section/utils.js

+ * @param {number} totalGroups - Total number of groups.
+ * @returns {string} The percentage of groups, each group has the same value.
+ */
+const getGroupPercentage = (totalGroups) => `${totalGroups === 0 ? 0 : Math.floor(100 / totalGroups)}%`;


Suggested change

const getGroupPercentage = (totalGroups) => `${totalGroups === 0 ? 0 : Math.floor(100 / totalGroups)}%`;

const getGroupPercentage = (totalGroups) => totalGroups === 0 ? '0%' : `${Math.floor(100 / totalGroups)}%`;

ruzniaievdm · 2024-02-28T16:54:02Z

@ruzniaievdm The feature with percentage looks pretty cool!!! I have a couple of suggestions:

Should the font size of error text be the same?

3. In the current implementation, the user can create a group with an empty value (if you add spaces instead of text, the groups can be saved)

Good catch for both, thanks!

monteri · 2024-02-29T11:15:05Z

src/group-configurations/GroupConfigurations.scss

@@ -26,7 +26,7 @@
    align-content: center;
    justify-content: space-between;

-    .configuration-card-header__button {
+    .configuration-card-header-button {


Previously it was OK for class names. Only change for data-test-id is required to the word-word-word-word pattern. classNames should be with double underscores as you had previously.

Sorry for confusion

reverted changed

monteri · 2024-02-29T11:35:00Z

src/group-configurations/experiment-configurations-section/ExperimentCardGroup.jsx

@@ -10,8 +10,8 @@ const ExperimentCardGroup = ({ groups }) => {
    <Stack className="mb-3">
      {groups.map((item) => (
        <div
-          className="configuration-card-content__experiment-stack"
-          data-testid="configuration-card-content__experiment-stack"
+          className="configuration-card-content-experiment-stack"


Here also configuration-card-content__experiment-stack

codecov-commenter · 2024-02-29T11:37:24Z

Codecov Report

Attention: Patch coverage is 62.50000% with 90 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (ts-develop@68d7533). Click here to learn what that means.

Files	Patch %	Lines
src/group-configurations/data/thunk.js	0.00%	34 Missing ⚠️
src/group-configurations/hooks.jsx	11.76%	15 Missing ⚠️
src/group-configurations/data/slice.js	0.00%	13 Missing and 1 partial ⚠️
src/group-configurations/data/api.js	0.00%	9 Missing ⚠️
...periment-configurations-section/ExperimentCard.jsx	82.85%	5 Missing and 1 partial ⚠️
...ations/content-groups-section/ContentGroupCard.jsx	70.00%	3 Missing ⚠️
...up-configurations/content-groups-section/index.jsx	50.00%	2 Missing ⚠️
...periment-configurations-section/ExperimentForm.jsx	92.00%	2 Missing ⚠️
...nt-configurations-section/ExperimentFormGroups.jsx	88.88%	2 Missing ⚠️
...ations/experiment-configurations-section/index.jsx	83.33%	2 Missing ⚠️
... and 1 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@              Coverage Diff              @@
##             ts-develop     #188   +/-   ##
=============================================
  Coverage              ?   89.07%           
=============================================
  Files                 ?      680           
  Lines                 ?    11500           
  Branches              ?     2436           
=============================================
  Hits                  ?    10244           
  Misses                ?     1216           
  Partials              ?       40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… Groups (#188) * feat: [AXIMST-93, 99, 105] Group configuration - Experiment Groups * fix: [AXIMST-518, 537] Group configuration - resolve bugs * fix: review discussions * fix: revert classname case

ruzniaievdm force-pushed the ruzniaievdm/feat/experiment-configuration branch from 4ea5971 to a1500ed Compare February 26, 2024 17:49

feat: [AXIMST-93, 99, 105] Group configuration - Experiment Groups

a162c3b

ruzniaievdm force-pushed the ruzniaievdm/feat/experiment-configuration branch from a1500ed to a162c3b Compare February 26, 2024 17:55

ruzniaievdm self-assigned this Feb 26, 2024

fix: [AXIMST-518, 537] Group configuration - resolve bugs

5ee2589

ruzniaievdm requested review from monteri, khudym, PKulkoRaccoonGang, vladislavkeblysh and ihor-romaniuk February 27, 2024 16:19

ruzniaievdm marked this pull request as ready for review February 27, 2024 16:23

ruzniaievdm changed the title ~~feat: [AXIMST-93, 99, 105] Group configuration - Experiment Groups~~ feat: [AXIMST-93, 99, 105, 518, 537] Group configuration - Experiment Groups Feb 27, 2024

PKulkoRaccoonGang requested changes Feb 27, 2024

View reviewed changes

PKulkoRaccoonGang added the group-configuration label Feb 27, 2024

monteri reviewed Feb 28, 2024

View reviewed changes

fix: review discussions

30d0021

ruzniaievdm requested review from monteri and PKulkoRaccoonGang February 28, 2024 18:35

PKulkoRaccoonGang approved these changes Feb 28, 2024

View reviewed changes

monteri reviewed Feb 29, 2024

View reviewed changes

fix: revert classname case

05362bb

ruzniaievdm force-pushed the ruzniaievdm/feat/experiment-configuration branch from 740dc5e to 05362bb Compare February 29, 2024 11:40

monteri merged commit 819e60b into ts-develop Feb 29, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [AXIMST-93, 99, 105, 518, 537] Group configuration - Experiment Groups #188

feat: [AXIMST-93, 99, 105, 518, 537] Group configuration - Experiment Groups #188

ruzniaievdm commented Feb 26, 2024 •

edited

Loading

PKulkoRaccoonGang left a comment •

edited

Loading

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

monteri Feb 28, 2024

ruzniaievdm Feb 28, 2024

ruzniaievdm commented Feb 28, 2024

monteri Feb 29, 2024

monteri Feb 29, 2024

ruzniaievdm Feb 29, 2024

monteri Feb 29, 2024

codecov-commenter commented Feb 29, 2024

	* Delete exists experimental configuration from the course.
	* Delete existing experimental configuration from the course.

	const cardTitle = getByTestId('configuration-card-header__button');
	const cardTitle = getByTestId('configuration-card-header-button');

	data-testid="configuration-card-content__experiment-stack"
	data-testid="configuration-card-content-experiment-stack"

	<Form.Group className="mt-3 form-group-configuration" isInvalid={!!errors.name}>
	<Form.Group className="mt-3 configuration-form-group" isInvalid={!!errors.name}>

	const allGroupNameAreUnique = (groups) => {
	const allGroupNamesAreUnique = (groups) => {

	const getGroupPercentage = (totalGroups) => `${totalGroups === 0 ? 0 : Math.floor(100 / totalGroups)}%`;
	const getGroupPercentage = (totalGroups) => totalGroups === 0 ? '0%' : `${Math.floor(100 / totalGroups)}%`;

feat: [AXIMST-93, 99, 105, 518, 537] Group configuration - Experiment Groups #188

feat: [AXIMST-93, 99, 105, 518, 537] Group configuration - Experiment Groups #188

Conversation

ruzniaievdm commented Feb 26, 2024 • edited Loading

The main group configuration page

There were added actions to handle group configuration:

Create

Edit

Delete

PKulkoRaccoonGang left a comment • edited Loading

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

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

ruzniaievdm commented Feb 28, 2024

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

codecov-commenter commented Feb 29, 2024

Codecov Report

ruzniaievdm commented Feb 26, 2024 •

edited

Loading

PKulkoRaccoonGang left a comment •

edited

Loading