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

feat(interview): apply to all [2] #1125

Merged
merged 11 commits into from
Jan 10, 2025
Merged

feat(interview): apply to all [2] #1125

merged 11 commits into from
Jan 10, 2025

Conversation

jochenklar
Copy link
Member

@jochenklar jochenklar commented Aug 16, 2024

This PR adds the possibility to make projects to all users (#152)

  • Visible projects can be accessed read only and can be used as template
  • In a multi site setup, project visibility can be restricted to sites
  • Project visibility can also be restricted to groups
  • For now site manager can set the visibility, but this might be changed in the future

To that purpose, it adds a copyValue action to the new interface, which can be used to copy values to different tabs of a page.

It also makes the add value button for questions a bit smaller.

@jochenklar jochenklar self-assigned this Aug 16, 2024
@jochenklar jochenklar added this to the RDMO 2.3.0 milestone Aug 16, 2024
@jochenklar jochenklar force-pushed the interview-visibility branch from 199863c to be2dea7 Compare August 25, 2024 16:09
@jochenklar jochenklar force-pushed the interview-apply-to-all branch from 5c65410 to 498ae19 Compare August 25, 2024 16:14
Copy link
Member

@MyPyDavid MyPyDavid left a comment

Choose a reason for hiding this comment

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

this is front-end only, so it does not require any tests 😶‍🌫️

Ive tried out answering some questions and applying the answers to other datasets, it functions intuitively.

One problem I found for the multiple text answer types, where I could not see the button "apply to all" anymore after filling-out and removing again. Are these values not properly compared? And should I also be able to apply-to-all after i just add another entry or not?

apply-to-all-and-2-multiple-text-widgets

Another thing is about the radio buttons, they are floating over, but it's not for this PR I guess, see picture.

apply-to-all-and-1

@jochenklar
Copy link
Member Author

The radio button problem should be fixed in 24f6831.

@jochenklar
Copy link
Member Author

About the apply to all, it works like expected (by me 😄 ). It apears only if the question in at least one of the "sibling" sets has no or only empty values. Maybe we discuss how the feature should work when showing it to more users.

@jochenklar jochenklar changed the title Interview apply to all feature(interview): apply to all [2] Nov 19, 2024
@jochenklar jochenklar changed the title feature(interview): apply to all [2] feat(interview): apply to all [2] Nov 19, 2024
@jochenklar jochenklar force-pushed the interview-visibility branch from be2dea7 to 493f4b5 Compare December 6, 2024 08:53
Base automatically changed from interview-visibility to 2.3.0 December 12, 2024 14:29
@jochenklar jochenklar force-pushed the interview-apply-to-all branch from 498ae19 to 5ec0b86 Compare December 12, 2024 14:52
@jochenklar jochenklar marked this pull request as ready for review December 12, 2024 14:54
@jochenklar jochenklar requested a review from MyPyDavid December 12, 2024 14:54
@jochenklar
Copy link
Member Author

Rebased and good to go!

@MyPyDavid
Copy link
Member

So, with the behaviour of the checkboxes I can only add selected checkboxes and not remove unselected checkboxes when I apply to all.
Is this the desired behaviour? For consistency I would expect that the whole set of selected und unselected checkboxes will be applied to another dataset. I know that the unselected Values are deleted and just gone right, but would it be possible to implement this via the React state?

@jochenklar
Copy link
Member Author

The behavior was a bit odd, because of a bug. The idea was that values are not copied when any value exists "on the receiving side" (which is not what you describe, lets discuss this later in a call or so). initValue did create an empty value for checkboxes as well, thats why the copy arrow apeared (for a first set) just after creating a second set, but not after a reload. I fixed this and then refactored the check in QuestionsCopyValue to check for each "other" set if there are empty values or none at all.

sets.filter((set) => set.set_prefix === setPrefix).forEach((set) => {
element.elements.filter((e) => (e.model === 'questions.question')).forEach((question) => {
// check if there is any value for this question and set
if (isNil(values.find((value) => (
(value.attribute === question.attribute) &&
(value.set_prefix == set.set_prefix) &&
(value.set_index == set.set_index)
Copy link
Member

Choose a reason for hiding this comment

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

do these need to be strict === as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

in principle == would be ok, I guess but I will change it (JS is weird).

@MyPyDavid
Copy link
Member

from local testing the interview, now the "Apply this answer to all tabs where this question is empty" behaves accordingly.

As a side note, from a UX, I maybe would expect a button to erase (uncheck) all check-boxes but that's not for this PR.

@jochenklar
Copy link
Member Author

I added the erase button to the checkboxes, it was not to complicated. I don't know why I did not add it in the first place.

@MyPyDavid MyPyDavid self-requested a review January 9, 2025 10:13
@MyPyDavid
Copy link
Member

MyPyDavid commented Jan 9, 2025

When reopening the interview in an existing project, at a checkbox question I got the "existing value" conflict validation error.

conflict: [ "An existing value for this attribute/set_prefix/set_index/collection_index was found." ]

but it was only initially, after that I tried to make another Dataset and could check the boxes in there.

PS
For a newly created project, it seems to be fine.

@MyPyDavid
Copy link
Member

MyPyDavid commented Jan 9, 2025

When I apply here to all

Set I
Yes/No

    [http://example.com/terms/questions/catalog/conditions/set/bool](http://localhost:8000/management/questions/101/)
    [http://example.com/terms/domain/conditions/set/bool](http://localhost:8000/management/attributes/114/)

then it also applies to the next question

Set II
Yes/No

    [http://example.com/terms/questions/catalog/conditions/set_set/bool](http://localhost:8000/management/questions/102/)
    [http://example.com/terms/domain/conditions/set/bool](http://localhost:8000/management/attributes/114/)

Is that a bug?

@jochenklar
Copy link
Member Author

No, its the same attribute. However, you found a different bug 🐛. The Set II does not work because resolving the condition for the questionset was not triggered. It should be fixed now.

@jochenklar
Copy link
Member Author

About the other thing, maybe there was a faulty value in your database from tests before.

Copy link
Member

@MyPyDavid MyPyDavid 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, thanks!

@jochenklar jochenklar merged commit 0ddc19d into 2.3.0 Jan 10, 2025
19 checks passed
@jochenklar jochenklar deleted the interview-apply-to-all branch January 10, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants