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

Integration of the module "Transfer" #51

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ClaireLozano
Copy link
Contributor

@ClaireLozano ClaireLozano commented Mar 23, 2018

Hilary pull request : oaeproject/Hilary#1326
3akai-ux pull request : oaeproject/3akai-ux#4124


This change is Reviewable

@brecke
Copy link
Member

brecke commented Apr 6, 2018

Also, don't forget that we'll soon be running node6+ so feel free to use ES2015+ here in stuff like avoiding repeating the module.exports chained assignment and also lets and consts etc


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


lib/api.transfer.js, line 41 at r1 (raw file):

    };

    RestUtil.RestRequest(RestContext, '/api/transfer/create', 'post', params, function(err, transfer) {

same comment as before: this should be a POST + a resource-only URL /api/transfer


lib/api.transfer.js, line 59 at r1 (raw file):

 */
var getTransferById = module.exports.getTransferById = function(RestContext, originalUserId, callback) {
    RestUtil.RestRequest(RestContext, '/api/transfer/getTransferById/'+ RestUtil.encodeURIComponent(originalUserId), 'get', {'originalUserId' : originalUserId} , function(err, transfer) {

same here, check review on 3akai


lib/api.transfer.js, line 87 at r1 (raw file):

    };

    RestUtil.RestRequest(RestContext, '/api/transfer/makeTransfer', 'post', params, function(err, managers) {

and here


lib/api.transfer.js, line 113 at r1 (raw file):

    };
    
    RestUtil.RestRequest(RestContext, '/api/transfer/deleteTransfer', 'POST', params, function(err) {

and here


Comments from Reviewable

Copy link
Contributor Author

@ClaireLozano ClaireLozano left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @brecke)


lib/api.transfer.js, line 41 at r1 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same comment as before: this should be a POST + a resource-only URL /api/transfer

Done.


lib/api.transfer.js, line 59 at r1 (raw file):

Previously, brecke (Miguel Laginha) wrote…

same here, check review on 3akai

Done.


lib/api.transfer.js, line 87 at r1 (raw file):

Previously, brecke (Miguel Laginha) wrote…

and here

Done.


lib/api.transfer.js, line 113 at r1 (raw file):

Previously, brecke (Miguel Laginha) wrote…

and here

Done.

Copy link
Contributor Author

@ClaireLozano ClaireLozano left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @brecke)

Copy link
Member

@brecke brecke left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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