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

Feature/get organizations from ror api #416

Merged
merged 5 commits into from
Aug 11, 2021

Conversation

saulipurhonen
Copy link
Contributor

@saulipurhonen saulipurhonen commented Aug 9, 2021

Description

Forms with organisation property in form schema are treated as autocomplete fields. Data is fetched from ROR API.

Related issues

Closes #176

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

  • Use MaterialUI autocomplete field if field name is among autoCompleteIdentifiers.
  • More straightforward form data assertion and form reset. Helps when rendering edited form with autocomplete field.
  • Search ROR API for matching organisation name
  • Autocomplete + tooltip icon combination styling and positioning
  • Jest tests with Mock Service Worker library
  • Fixed E2E tests to work with autocomplete field

Testing

  • Unit Tests
  • Integration Tests

Get data from ROR API

Autocomplete with options from ROR API
@saulipurhonen saulipurhonen force-pushed the feature/get-organizations-from-ror-api branch 2 times, most recently from 1831154 to 83e7f5d Compare August 10, 2021 06:25
@saulipurhonen saulipurhonen force-pushed the feature/get-organizations-from-ror-api branch from 83e7f5d to 6723106 Compare August 10, 2021 08:56
@saulipurhonen saulipurhonen requested review from hannyle, lilachic and blankdots and removed request for hannyle August 10, 2021 09:02
@saulipurhonen saulipurhonen force-pushed the feature/get-organizations-from-ror-api branch from eea2855 to 57f5a32 Compare August 10, 2021 09:48
@saulipurhonen saulipurhonen force-pushed the feature/get-organizations-from-ror-api branch from 57f5a32 to eb07c57 Compare August 10, 2021 10:12
@saulipurhonen saulipurhonen marked this pull request as ready for review August 10, 2021 10:24
Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

seems to provide desired functionality, we can improve styles after we introduce new styles
image

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

maybe to the tooltip we can add that we are using ror.org for a standardised list of institutions, that would be ideal, if possible

@saulipurhonen
Copy link
Contributor Author

maybe to the tooltip we can add that we are using ror.org for a standardised list of institutions, that would be ideal, if possible

Sounds good to me. Would next sentence fit: Organisations provided by ror.org? And this text after the current description?

@blankdots
Copy link
Contributor

yes sounds ok, if we can make ror.org hyperlink even better

@saulipurhonen
Copy link
Contributor Author

yes sounds ok, if we can make ror.org hyperlink even better

Hyperlink would need tooltip to stay open when user moves cursor out from icon. MUI has option for interactive tooltips, I'll look into it.

Copy link
Contributor

@hannyle hannyle 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!
I think we can also mock the organisation data with cypress, I'm curious is there any other good reasons that we have it in unit test here?

@blankdots
Copy link
Contributor

I think we can also mock the organisation data with cypress, I'm curious is there any other good reasons that we have it in unit test here?

that is a fair point

@saulipurhonen
Copy link
Contributor Author

Looks good to me!
I think we can also mock the organisation data with cypress, I'm curious is there any other good reasons that we have it in unit test here?

Main reason for testing organisation data for autocomplete in unit test was to reduce weight on E2E CI pipeline. To be honest I'm getting a bit anxious when Cypress CI pipeline runs and fails multiple times with inconsistent reasons :/

However Cypress test for this feature could be handy since unit test doesn't test database interaction.

@blankdots
Copy link
Contributor

Cypress CI pipeline runs and fails multiple times with inconsistent reasons

yes, i am hopeful we can create an issue and spend some time on optimising the cypress test we can make them a tad better, but yes it is frustrating :(

@blankdots blankdots merged commit c8e1d17 into develop Aug 11, 2021
@blankdots blankdots deleted the feature/get-organizations-from-ror-api branch August 11, 2021 07:50
@blankdots blankdots mentioned this pull request Aug 11, 2021
5 tasks
@blankdots blankdots mentioned this pull request Oct 12, 2021
3 tasks
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.

4 participants