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] Use API for candidate creation and swal for success #7755

Merged
merged 6 commits into from
Oct 27, 2022

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Oct 29, 2021

This updates the candidate creation on the new profile page to
use the LORIS API instead of duplicating the logic in PHP. The form
already collects the exact data that a POST request to the API
requires, but submits it to a different endpoint in a form encoding
instead of a json encoding.

At the same time, the logic for the response is simplified by using
a swal instead of a (very empty looking) new page. swals are already
used for errors in the module, just not success. This change simplifies
both the code and the UX.

@driusan driusan added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Oct 29, 2021
@driusan
Copy link
Collaborator Author

driusan commented Oct 29, 2021

Blocked by the release. Set it to "Draft" so that it doesn't accidentally get merged beforehand.

submitDisabled: false,
});
}
});
});
} else {
resp.json().then((message) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will throw a parse error when an html page is returned by the backend (session expired, forbidden, 500, etc)
In those cases, It might be relevant to fire the swal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which part? the resp.json() or changing message to message.error? And why does forbidden return html? (It didn't seem to when I was testing..)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say thta it is because the class is extending \NDB_Form and not Endpoint. It's wraped by a UserPage Decorator middleware that only prints html. Regardless of specifics, there are cases where html will be returned and resp.json() will not be happy. ONe way of handling that is to put the

this.setState({submitDisabled: false});
swal.fire('Error!', message.error, 'error');

parts in the catch; and throw resp.json().message in the else.

@xlecours
Copy link
Contributor

Nice!
Additionally, in modules/new_profile/php/new_profile.class.inc, private $_error; can be removed.

@driusan
Copy link
Collaborator Author

driusan commented Oct 29, 2021

looks like that's also true of _pscid and _candID and all the namespaces (which phan was complaining about).. should be fixed in the last commit.

@driusan driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Mar 29, 2022
@driusan driusan marked this pull request as ready for review March 29, 2022 18:14
@driusan
Copy link
Collaborator Author

driusan commented Mar 29, 2022

@xlecours now that the release is out I removed the blocked tag and rebased this, can you re-review it?

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

I see a mixte of 2 and 4 spaces indentation in the js files.
The resp.json() potential parse error is still not handled. A swal showing failure would be better than a console.error I think.
It's failing CI
There was 1 error:

  1. NewProfileTestIntegrationTest::testNewProfileCreateCandidate
    Facebook\WebDriver\Exception\NoSuchElementException: Unable to locate element: #lorisworkspace > fieldset > div > div > p:nth-child(1)
    Other than that it's great :)

This updates the candidate creation on the new profile page to
se the LORIS API instead of duplicating the logic in PHP. The form
already collects the exact data that a POST request to the API
requires, but submits it to a different endpoint in a form encoding
instead of a json encoding.

At the same time, the logic for the response is simplified by using
a swal instead of a (very empty looking) new page. swals are already
used for errors in the module, just not success. This change simplifies
both the code and the UX.
@driusan
Copy link
Collaborator Author

driusan commented Oct 25, 2022

@xlecours I've fixed the indentation, added a catch to the code paths that don't have a catch for the resp.json(), fixed an error in the API that was causing it to return non-JSON and, if the gods are good, fixed the failing test by checking the swal title instead of an element in the body. (Still needs to run on GitHub to be sure that it's working..)

@driusan driusan added Release: Add to release notes PR whose changes should be highlighted in the release notes Category: Feature PR or issue that aims to introduce a new feature Area: UI PR or issue related to the user interface Cleanup PR or issue introducing/requiring at least one clean-up operation labels Oct 25, 2022
Copy link
Contributor

@xlecours xlecours 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 good.

@xlecours
Copy link
Contributor

@driusan I haven't tested it because I am in the middle of other things.

@xlecours
Copy link
Contributor

@driusan Tested now. it's working all right.

@driusan driusan merged commit 3792d2a into aces:main Oct 27, 2022
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
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 Category: Feature PR or issue that aims to introduce a new feature Cleanup PR or issue introducing/requiring at least one clean-up operation Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants