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

[new_profile] Auto-select only site option #6156

Merged
merged 10 commits into from
Aug 19, 2020

Conversation

zaliqarosli
Copy link
Contributor

@zaliqarosli zaliqarosli commented Mar 11, 2020

Brief summary of changes

Resolves issue #5380

Testing instructions (if applicable)

(make sure to run make dev when you switch branches)

  1. On main branch, on a user account affiliated with multiple sites, go to New Profile module and create a new candidate. You are required to choose a site on the form.
  2. Repeat the above step but on a user account affiliated with only one site. Notice that the 'Site' field does not appear and on successful creation of the candidate, the candidate is created for the correct site.
  3. On this PR branch, repeat the above steps. With a multiple-site user account, there should be no changes. With a single-site user account, notice that the 'Site' field appears and that the only site option is auto-selected. You should get no errors creating the new candidate.
  4. Finally, test a couple of different select elements, both with multiple options and single options, in Loris to ensure that no unexpected behaviours appear. (select elements within module menu filters should not be affected because autoSelect is set to false there)

Link(s) to related issue(s)

jsx/Form.js Outdated Show resolved Hide resolved
@zaliqarosli
Copy link
Contributor Author

hmm im running into the issue of the form data object not populating with the default option if its automatically selected due to onUserInput never triggering

@zaliqarosli zaliqarosli added the State: Needs work PR awaiting additional work by the author to proceed label Mar 13, 2020
@maltheism
Copy link
Member

@zaliqarosli I got it to work by adding a few changes to the SelectElement component.
I gave the SelectElement a state:

this.state = {
      selected: null,
};

then made this change:

// Add empty option
// If only one option exists and element is required, don't add empty option so that the only available
// option is selected by default
if ((this.props.emptyOption && Object.keys(this.props.options).length != 1)
    || !this.props.required) {
        emptyOptionHTML = <option></option>;
} else if (this.state.selected === null) {
    const selected = this.props.options[
        Object.keys(this.props.options)[0]
    ];
    this.setState({selected: selected});
    this.props.onUserInput(this.props.name, selected, null, 'select');
}

I won't be offended if you go with something different because it isn't the most elegant solution.

@zaliqarosli
Copy link
Contributor Author

@maltheism hehe thank you ;) i was going the same way but was also unsure about the hackiness of it

@zaliqarosli zaliqarosli changed the base branch from master to main July 21, 2020 18:02
have empty option if field not required
@zaliqarosli zaliqarosli force-pushed the 2020-03-11-MakeOneOptionDefault branch from bfd7bd5 to ee1a1fc Compare August 4, 2020 19:57
@zaliqarosli zaliqarosli changed the title [jsx/Form] SelectElement - Automatically select only option by default [new_profile] Auto-select only site option Aug 4, 2020
@zaliqarosli zaliqarosli removed the State: Needs work PR awaiting additional work by the author to proceed label Aug 4, 2020
@zaliqarosli zaliqarosli closed this Aug 5, 2020
@zaliqarosli zaliqarosli reopened this Aug 5, 2020
@HenriRabalais
Copy link
Collaborator

HenriRabalais commented Aug 10, 2020

I know @maltheism gave a suggestion above, but I thought I would give my own solution to this issue that I use in the Biobank that doesn't involve giving the selectElement a state.

One of the eventual issues I reached when implementing it is that when there is only 1 value and the empty option is removed, although it appears that the only remaining option is selected, the onUserInput function is never triggered and so therefore the value is never passed to the parent component. I see you've created a workaround in the NewProfileIndex.js to deal with this, but I believe we will run into this issue with other modules and filters when they only have one value. I suggest doing this process in the selectElement itself so that it's taken care of across all modules.

What I did is I created a new prop called autoSelect and set the value to default to true. I then triggered the onUserInput function on component mount and update (in case the number of options changed to 1, in a dynamic form for example) if the options object had a length value of 1.

Let me know what you think!

/**
 * Select Component
 * React wrapper for a simple or 'multiple' <select> element.
 */
class SelectElement extends Component {
  constructor(props) {
    super(props);
    this.handleChange = this.handleChange.bind(this);
  }

  componentDidMount() {
    const optionsArray = Object.keys(this.props.options);
    if (this.props.autoSelect && optionsArray.length === 1) {
      this.props.onUserInput(this.props.name, optionsArray[0]);
    }
  }

  componentDidUpdate(prevProps) {
    const options = Object.keys(this.props.options);
    const prevOptions = Object.keys(prevProps.options);
    if (options.length !== prevOptions.length ||
        !options.every((v, i) => v === prevOptions[i])) {
      if (this.props.autoSelect && options.length === 1) {
        this.props.onUserInput(this.props.name, options[0]);
      }
    }
  }

@maltheism
Copy link
Member

@HenriRabalais I had to comment that your solution is very cool!

@zaliqarosli
Copy link
Contributor Author

zaliqarosli commented Aug 10, 2020

@HenriRabalais that solution looks good to me! the only thing that i would change though is to maybe make the default value false? i noticed when testing this PR that we wouldn't want to auto select the only option on menu filters (we always want to load the page with no filters selected). we would only really want to auto-select on forms where the field is required. we could either add the this.props.required check to the if statements here or make autoSelect an opt-in option. which way do you think we should go?

@HenriRabalais
Copy link
Collaborator

HenriRabalais commented Aug 10, 2020

@zaliqarosli Ah, yes that's a good point. It would be so nice if we could just default to true! I think opting in could work. Now that I think about it though, since the filter fields are generated by the Filter.js class, the autoSelect prop could be set to false there for all the SelectElements.

e.g.

renderFilterFields() {
    return this.props.fields.reduce((result, field) => {
      const filter = field.filter;
      if (filter && filter.hide !== true) {
        let element;
        switch (filter.type) {
        case 'text':
          element = <TextboxElement key={filter.name}/>;
          break;
        case 'select':
          element = (
            <SelectElement
              key={filter.name}
              options={filter.options}
              sortByValue={filter.sortByValue}
              autoSelect={false}
            />
          );

@zaliqarosli
Copy link
Contributor Author

zaliqarosli commented Aug 11, 2020

@HenriRabalais that's a good option! are all menu filters reactified for sure? edit: i guess it doesn't matter because this change would only affect reactified ones anyways!

@HenriRabalais
Copy link
Collaborator

@zaliqarosli Yeah exactly! If they're reactified they should be using the FilterableDatatable.js component.

@zaliqarosli
Copy link
Contributor Author

@HenriRabalais ready for re-review

HenriRabalais
HenriRabalais previously approved these changes Aug 11, 2020
Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ridz1208
Copy link
Collaborator

@HenriRabalais could you test it out as well ?

jsx/Form.js Outdated Show resolved Hide resolved
jsx/Form.js Outdated Show resolved Hide resolved
@zaliqarosli
Copy link
Contributor Author

@HenriRabalais @driusan ready for re-review!

Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

please add content to the CHANGELOG file

@HenriRabalais HenriRabalais added the Passed manual tests PR has been successfully tested by at least one peer label Aug 13, 2020
@ridz1208 ridz1208 dismissed their stale review August 17, 2020 16:12

changes done

@driusan driusan merged commit 34d1d5a into aces:main Aug 19, 2020
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 20, 2020
zaliqarosli added a commit to zaliqarosli/Loris that referenced this pull request Sep 3, 2020
driusan pushed a commit that referenced this pull request Sep 8, 2020
Adjust test plan to reflect new behavior.

See #6156
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
Adjust test plan to reflect new behavior.

See aces#6156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[new_profile] If user assigned to 1 site, unhide site field and auto-populate default site
5 participants