feat(interview): Copy dataset [3] #1128

jochenklar · 2024-08-20T12:59:39Z

This PR adds the possibility to copy tabs (i.e. datasets) or better to create a new tab with all values from the current tab. It works a bit like deleting a set. If a set_value is present (e.g. project/dataset/id), the catalog is used to get all values for the current set and copy them, along with a new set_value. For # style sets without attributes this is done in the front end. There is some black magic involved when the set_prefix of values is updated with the set_index of the new set.

MyPyDavid

think it looks good, thanks! 🙏

Signed-off-by: David Wallace <[email protected]>

jochenklar · 2025-01-10T14:34:27Z

Rebased and good to go.

jochenklar · 2025-01-10T14:36:06Z

rdmo/projects/assets/js/interview/actions/interviewActions.js

+
+          if (value.set_prefix == set.set_prefix) {
+            value.set_index = set.set_index
+          } else {


@MyPyDavid This is complicated, can you check if this is understandable to you. I just fixed a bug here. This front-end part is for copying blocks/questionset (and tabs/pages without attribute).

ok, yes, I can not make any cheese out of this but the comments help ;).
I've asked a LLM to explain it to me..
You don't want to split it a bit more into functions with meaningful names?

function adjustSetPrefix(value, set, newSetPrefixDepth) { value.set_prefix = value.set_prefix.split('|').map((segment, idx) => { return idx === newSetPrefixDepth ? set.set_index : segment; }).join('|'); return value; }

export function copySet(currentSet, currentSetValue, attrs) { const pendingId = `copySet/${currentSet.set_prefix}/${currentSet.set_index}`; return (dispatch, getState) => { dispatch(addToPending(pendingId)); dispatch(copySetInit()); const set = createNewSet(attrs); // Encapsulates set creation logic const value = prepareSetValue(attrs); // Encapsulates value preparation logic if (isNil(value)) { handleSetWithoutAttribute(dispatch, getState, pendingId, currentSet, set, finalizeCopySet(dispatch, getState)); } else { handleSetWithAttribute(dispatch, pendingId, currentSetValue, value, finalizeCopySet(dispatch, getState)); } }; }

I think I will keep it like it is. Moving dispatch into functions somehow feels wrong. To me it is actually harder to read with the functions.

jochenklar · 2025-01-10T14:37:34Z

rdmo/projects/viewsets.py

+        set_value_serializer.is_valid(raise_exception=True)
+        set_value = set_value_serializer.save()
+        set_value_data = set_value_serializer.data
+


This is the corresponding part in the backend for tabs/pages with an attribute. The problem before was that set_value.set_prefix.split('|') for '' and '0' is both 1. I hope I fixed it now.

ok, wanna handle each value maybe in a separate function inside of the copy_set method?

def adjust_set_prefix(value, set_value, set_prefix_length): if value.set_prefix == set_value.set_prefix: value.set_index = set_value.set_index else: value.set_prefix = '|'.join([ str(set_value.set_index) if (index == set_prefix_length) else part for index, part in enumerate(value.set_prefix.split('|')) ]) return value

ok, after some thought, I added a compute_set_prefix_from_set_value function for the else part. Mainly because I want some parametrized tests for it.

MyPyDavid

Overall I think that the current implementation behaves as expected.
I have made only few optional code refactoring remarks.

Somethings about the UX of the tabs of the datasets, they should behave similar to tabs in a browser right?

on deleting a dataset, it should activate the one to the right first when it's possible
for UI design, should the delete icon not be inside of the activated tab? (or on all tabs??)
in the delete modal, it should mention the name/label of the tab that will be deleted

During the interview, when applying to all or making the copy, there are multiple requests to /navigation and /progress. Could this maybe be optimized? I think it calls get requests for each dataset that is updated. Don't know if this could be handled somehow in a single request to each endpoint.

MyPyDavid · 2025-01-16T08:30:43Z

rdmo/projects/assets/js/interview/actions/interviewActions.js

+
+          if (value.set_prefix == set.set_prefix) {
+            value.set_index = set.set_index
+          } else {


ok, yes, I can not make any cheese out of this but the comments help ;).
I've asked a LLM to explain it to me..
You don't want to split it a bit more into functions with meaningful names?

function adjustSetPrefix(value, set, newSetPrefixDepth) { value.set_prefix = value.set_prefix.split('|').map((segment, idx) => { return idx === newSetPrefixDepth ? set.set_index : segment; }).join('|'); return value; }

export function copySet(currentSet, currentSetValue, attrs) { const pendingId = `copySet/${currentSet.set_prefix}/${currentSet.set_index}`; return (dispatch, getState) => { dispatch(addToPending(pendingId)); dispatch(copySetInit()); const set = createNewSet(attrs); // Encapsulates set creation logic const value = prepareSetValue(attrs); // Encapsulates value preparation logic if (isNil(value)) { handleSetWithoutAttribute(dispatch, getState, pendingId, currentSet, set, finalizeCopySet(dispatch, getState)); } else { handleSetWithAttribute(dispatch, pendingId, currentSetValue, value, finalizeCopySet(dispatch, getState)); } }; }

rdmo/projects/assets/js/interview/actions/interviewActions.js

MyPyDavid · 2025-01-16T08:52:54Z

rdmo/projects/managers.py

+
+        # collect the attributes of all questions of all pages or questionsets
+        # of this catalog, which have the attribute of this value
+        attributes = set()


suggestion for this:

attributes = {descendant.attribute for element in (catalog.pages + catalog.questions) if element.attribute == set_value.attribute for descendant in element.descendants}

Would be the same right? as a set comprehension

MyPyDavid · 2025-01-16T09:09:46Z

rdmo/projects/viewsets.py

+        set_value_serializer.is_valid(raise_exception=True)
+        set_value = set_value_serializer.save()
+        set_value_data = set_value_serializer.data
+


ok, wanna handle each value maybe in a separate function inside of the copy_set method?

def adjust_set_prefix(value, set_value, set_prefix_length): if value.set_prefix == set_value.set_prefix: value.set_index = set_value.set_index else: value.set_prefix = '|'.join([ str(set_value.set_index) if (index == set_prefix_length) else part for index, part in enumerate(value.set_prefix.split('|')) ]) return value

jochenklar · 2025-01-16T15:02:01Z

I would like to keep the icons where they are, I think it would be to crowded to add all the options to all tabs. (Makes more sense for a browser where you open and close tabs all the time.)

jochenklar · 2025-01-16T15:02:48Z

I added the name of the tab to the delete modal:

MyPyDavid · 2025-01-16T15:38:10Z

I would like to keep the icons where they are, I think it would be to crowded to add all the options to all tabs. (Makes more sense for a browser where you open and close tabs all the time.)

this is also fine with me! 👍

jochenklar · 2025-01-16T18:17:30Z

The remark with the /progress opened a can of worms. I now refactored copyValue quiet heavily. It now stores all values and then calls fetchNavigation, updateProgress and checks for a refresh. I think the original version could also been prone to race conditions, since a refresh could be triggered before all values are stored.

jochenklar · 2025-01-17T10:28:45Z

I also added 2 small fixes for the bugs we found in yesterdays meeting. Can you check again and approve?

MyPyDavid · 2025-01-17T10:38:46Z

I also added 2 small fixes for the bugs we found in yesterdays meeting. Can you check again and approve?

ok great, thanks so much! I'll check it today!

MyPyDavid · 2025-01-17T12:44:52Z

The remark with the /progress opened a can of worms. I now refactored copyValue quiet heavily. It now stores all values and then calls fetchNavigation, updateProgress and checks for a refresh. I think the original version could also been prone to race conditions, since a refresh could be triggered before all values are stored.

thanks, this looks smooth now!

MyPyDavid · 2025-01-17T12:47:32Z

Can the range slider also do apply to all? I can't see the icon for it..

And for file uploads, I would guess not right?

MyPyDavid

datasets can be copied smoothly now!

jochenklar · 2025-01-17T13:16:52Z

🙏

(the slider works, but it needs a value first)

jochenklar · 2025-01-17T14:54:02Z

I broke something else, good that we have tests 😏

MyPyDavid · 2025-01-17T15:05:55Z

I broke something else, good that we have tests 😏

yes, only for all this new interview stuff the front-end (playwright) tests are still missing! 🚨 😉

jochenklar self-assigned this Aug 22, 2024

jochenklar added this to the RDMO 2.3.0 milestone Aug 22, 2024

jochenklar force-pushed the interview-copy-dataset branch from cdb35a2 to 1082398 Compare August 22, 2024 07:34

jochenklar force-pushed the interview-apply-to-all branch from 5c65410 to 498ae19 Compare August 25, 2024 16:14

jochenklar force-pushed the interview-copy-dataset branch from 1082398 to 40a8e80 Compare August 25, 2024 17:26

MyPyDavid approved these changes Nov 8, 2024

View reviewed changes

jochenklar changed the title ~~Interview copy dataset~~ feature(interview): Copy dataset [3] Nov 19, 2024

jochenklar changed the title ~~feature(interview): Copy dataset [3]~~ feat(interview): Copy dataset [3] Nov 19, 2024

jochenklar force-pushed the interview-apply-to-all branch from 498ae19 to 5ec0b86 Compare December 12, 2024 14:52

Base automatically changed from interview-apply-to-all to 2.3.0 January 10, 2025 12:49

jochenklar and others added 5 commits January 10, 2025 14:03

Add copySet action to interview

e554c4c

Add copy_set function to ProjectValueViewSet

f5ce70f

Update tests

02916e9

Add copy_set function to question sets

8b316e4

style(projects): autofix typos

b453485

Signed-off-by: David Wallace <[email protected]>

jochenklar force-pushed the interview-copy-dataset branch from 01d9ef8 to b453485 Compare January 10, 2025 13:12

jochenklar added 3 commits January 10, 2025 15:19

Fix modal in QuestionSetCopySet component

1e957cb

Fix set_prefix computation in copySet

7d9e649

Fix copy_set action

3dfd383

jochenklar marked this pull request as ready for review January 10, 2025 14:34

jochenklar commented Jan 10, 2025

View reviewed changes

jochenklar requested a review from MyPyDavid January 13, 2025 09:49

MyPyDavid requested changes Jan 16, 2025

View reviewed changes

jochenklar added 2 commits January 16, 2025 15:53

Activate right tab when deleting tab

4074a84

Add name to PageHeadDeleteModal

0821cfc

jochenklar added 2 commits January 16, 2025 17:40

Fix PageHeadDeleteModal

0dcd7b7

Refactor copyValue so that fetchNavigation etc is only called once

ce9b331

MyPyDavid self-requested a review January 17, 2025 07:46

jochenklar added 4 commits January 17, 2025 10:19

Use set comprehension in ValueQuerySet

5f623b4

Add get_set_prefix_for_value_copy function with tests

6f7e57f

Fix select input component

23d4623

Fix visibility related issues

c619ff5

MyPyDavid approved these changes Jan 17, 2025

View reviewed changes

jochenklar added 2 commits January 17, 2025 14:31

Fix visibility field in ProjectSerializer

475b6ac

Add visibility to select_related for ProjectViewSet

6ce3b14

jochenklar merged commit 6fb05a5 into 2.3.0 Jan 17, 2025
19 checks passed

jochenklar deleted the interview-copy-dataset branch January 17, 2025 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(interview): Copy dataset [3] #1128

feat(interview): Copy dataset [3] #1128

jochenklar commented Aug 20, 2024

MyPyDavid left a comment

jochenklar commented Jan 10, 2025

jochenklar Jan 10, 2025

MyPyDavid Jan 16, 2025

jochenklar Jan 16, 2025 •

edited

Loading

jochenklar Jan 10, 2025

MyPyDavid Jan 16, 2025

jochenklar Jan 17, 2025 •

edited

Loading

MyPyDavid left a comment

MyPyDavid Jan 16, 2025

MyPyDavid Jan 16, 2025

MyPyDavid Jan 16, 2025

jochenklar commented Jan 16, 2025

jochenklar commented Jan 16, 2025

MyPyDavid commented Jan 16, 2025

jochenklar commented Jan 16, 2025

jochenklar commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

MyPyDavid left a comment

jochenklar commented Jan 17, 2025

jochenklar commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

feat(interview): Copy dataset [3] #1128

feat(interview): Copy dataset [3] #1128

Conversation

jochenklar commented Aug 20, 2024

MyPyDavid left a comment

Choose a reason for hiding this comment

jochenklar commented Jan 10, 2025

jochenklar Jan 10, 2025

Choose a reason for hiding this comment

MyPyDavid Jan 16, 2025

Choose a reason for hiding this comment

jochenklar Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

jochenklar Jan 10, 2025

Choose a reason for hiding this comment

MyPyDavid Jan 16, 2025

Choose a reason for hiding this comment

jochenklar Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

MyPyDavid left a comment

Choose a reason for hiding this comment

MyPyDavid Jan 16, 2025

Choose a reason for hiding this comment

MyPyDavid Jan 16, 2025

Choose a reason for hiding this comment

MyPyDavid Jan 16, 2025

Choose a reason for hiding this comment

jochenklar commented Jan 16, 2025

jochenklar commented Jan 16, 2025

MyPyDavid commented Jan 16, 2025

jochenklar commented Jan 16, 2025

jochenklar commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

MyPyDavid left a comment

Choose a reason for hiding this comment

jochenklar commented Jan 17, 2025

jochenklar commented Jan 17, 2025

MyPyDavid commented Jan 17, 2025

jochenklar Jan 16, 2025 •

edited

Loading

jochenklar Jan 17, 2025 •

edited

Loading