Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2018

2018-11-02T20:18:33Z

Dgupta10:

* [[CSC/ECE 517 Fall 2018- Project E1846. OSS Project Navy: Character Issues]]
* [[CSC/ECE 517 Fall 2018/OSS E1848 Write unit tests for assignment team.rb]]
* [http://wiki.expertiza.ncsu.edu/index.php/E1839_Review_Requirements_and_Thresholds CSC/ECE 517 Fall 2018 E1839 Review Requirements and Thresholds]
* [http://wiki.expertiza.ncsu.edu/index.php/E1848_Write_unit_tests_for_assignment_team CSC/ECE 517 Fall 2018 E1848 Write unit tests for assignment_team]
* [http://wiki.expertiza.ncsu.edu/index.php/E1835_Refactor_delayed_mailer_and_scheduled_task CSC/ECE 517 Fall 2018 E1835_Refactor_delayed_mailer_and_scheduled_task]

CSC/ECE 517 Fall 2018

2018-11-02T20:18:23Z

Dgupta10:

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-12-13T18:33:46Z

Dgupta10: /* Test plan */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The Problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:Design Document E1795.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:sso_diagram.jpg]]

== Implementation ==

We have successfully implemented the following components from the solution architecture.
* SSO App
The user will login to the Single Signon App and can ask for keys and enable the APIs for using Peerlogic services. Similarly the admin can approve the keys requested by the user, can view the information of the users and add APIs.

'''Credentials for admin login: '''

email: admin@gmail.com

password: admin

Please do not change the password of the admin so that another user doesn't get blocked while testing.

* SSO REST APIs
Once the key is approved by the admin, the Apps which use the Peerlogic service will use this keys to communicate with the REST Endpoints. A token will be generated from this which will be used by Peerlogic to authenticate and authorize the app.

=> To get the token: (Called by the app)

<pre>
URL : https://evening-refuge-64130.herokuapp.com/token/get?clientkey=y4E1irXZQfxVipxKo4llTnc5gdZtzP6OV3xfkS/ZP4U=

Method: GET

Response:
{
"client_id": "someID",
"token": "someToken",
"message": "OK",
"status": 200
}
</pre>

=> To authenticate and authorize: (Called by Peerlogic)

<pre>
URL : https://evening-refuge-64130.herokuapp.com/token/validate

Method: POST

Payload:
{
"token": "someToken",
"api": "Rainbow API"
}

Response:
{
"status": 200,
"message": "OK",
"clientname": "App"
}
</pre>

In cases where authentication fails, status will come as 401 with appropriate message giving the reason for failure.

* Demo App

We have made a Demo App to show how the API calls to Peerlogic work and token validation.
** A token is generated when you click on the 'RequestToken'. This token will be sent in the header of the request to Peerlogic while making an API call.
** Peerlogic verifies this token with SSO.
** Clicking on 'GetRainbowURL' generates a URL for that service through which we can access it.
** 'getGraph' shows the visual graph of Rainbow Service.
** Changing anything in the token will make it an invalid one and the service wouldn't be accessible. An error output will be displayed.

[[File:app.jpg]]

== Test plan ==

To test the app, follow the steps listed below:

1. Go to the single sign on app, login.

2. Create a new app, and request the client key for that.

3. Wait for admin approval.

4. If you have the admin credentials, login as admin, go to clients page, and approve the request you just generated.

5. Once the approval is done, copy the client key and keep it somewhere safe.

6. Now, to access the rainbow graph API, send a request to the SSO for token along with client key.

7. After the token is sent to you, you can use the token as Authorization header in the Rainbow graph API.

8. If the token is correct, you should be able to access the API, else an error will be thrown for unauthorized access.

== Challenges ==

There are a couple of challenges we have identified:

1. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

2. Making sure the database is secure: Since our database is going to store the encryption key and IV, it is of utmost importance that the database is secure and not prone to attacks.

== Future Scope ==

In future we would like to use multiple keys for encryption rather than just one constant key for a lifetime. These multiple keys will have a time to live. This will make the system more secure as now they will be different keys used at different times, so in case a key is leaked or even if it isn't, it won't last for much time, and soon a new key will be in place, bringing the system back to its initial secured position.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-12-13T18:28:30Z

Dgupta10: /* Test plan */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The Problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:Design Document E1795.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:sso_diagram.jpg]]

== Implementation ==

We have successfully implemented the following components from the solution architecture.
* SSO App
The user will login to the Single Signon App and can ask for keys and enable the APIs for using Peerlogic services. Similarly the admin can approve the keys requested by the user, can view the information of the users and add APIs.

'''Credentials for admin login: '''

email: admin@gmail.com

password: admin

Please do not change the password of the admin so that another user doesn't get blocked while testing.

* SSO REST APIs
Once the key is approved by the admin, the Apps which use the Peerlogic service will use this keys to communicate with the REST Endpoints. A token will be generated from this which will be used by Peerlogic to authenticate and authorize the app.

=> To get the token: (Called by the app)

<pre>
URL : https://evening-refuge-64130.herokuapp.com/token/get?clientkey=y4E1irXZQfxVipxKo4llTnc5gdZtzP6OV3xfkS/ZP4U=

Method: GET

Response:
{
"client_id": "someID",
"token": "someToken",
"message": "OK",
"status": 200
}
</pre>

=> To authenticate and authorize: (Called by Peerlogic)

<pre>
URL : https://evening-refuge-64130.herokuapp.com/token/validate

Method: POST

Payload:
{
"token": "someToken",
"api": "Rainbow API"
}

Response:
{
"status": 200,
"message": "OK",
"clientname": "App"
}
</pre>

In cases where authentication fails, status will come as 401 with appropriate message giving the reason for failure.

* Demo App

We have made a Demo App to show how the API calls to Peerlogic work and token validation.
** A token is generated when you click on the 'RequestToken'. This token will be sent in the header of the request to Peerlogic while making an API call.
** Peerlogic verifies this token with SSO.
** Clicking on 'GetRainbowURL' generates a URL for that service through which we can access it.
** 'getGraph' shows the visual graph of Rainbow Service.
** Changing anything in the token will make it an invalid one and the service wouldn't be accessible. An error output will be displayed.

[[File:app.jpg]]

== Test plan ==

To test the app, follow the steps listed below:

1. Go to the single sign on app, login.

2. Create a new app, and request the client key for that.

3. Wait for admin approval. (Admin will approve this request)

4. Once the approval is done, copy the client key and keep it somewhere safe.

5. Now, to access the rainbow graph API, send a request to the SSO for token along with client key.

6. After the token is sent to you, you can use the token as Authorization header in the Rainbow graph API.

7. If the token is correct, you should be able to access the API, else an error will be thrown for unauthorized access.

== Challenges ==

There are a couple of challenges we have identified:

1. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

2. Making sure the database is secure: Since our database is going to store the encryption key and IV, it is of utmost importance that the database is secure and not prone to attacks.

== Future Scope ==

In future we would like to use multiple keys for encryption rather than just one constant key for a lifetime. These multiple keys will have a time to live. This will make the system more secure as now they will be different keys used at different times, so in case a key is leaked or even if it isn't, it won't last for much time, and soon a new key will be in place, bringing the system back to its initial secured position.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-15T02:22:38Z

Dgupta10: /* Test plan */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The Problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:peerlogic.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:class_diagram.jpg]]

== Test plan ==

We will be testing our SSO app both manually and using automation using rspec.

Rspec tests will include:
1. Request a key, application should redirect to the main page with another listing added, with status as requested.

2. Admin approves a key, application should redirect to the main page with listing's status changed to approved and key generated and shown.

3. Admin declines a key, application should redirect to the main page with listing's status changed to declined.

4. User revokes a key, application should redirect to the main page with listing's status changed to revoked.

5. Clients send request, token is generated.

6. Client sends request with valid token, status sent 200 OK.

7. Client sends request with invalid token, status sent 401 Unauthorized.

There might be more tests added during the course of project development based on the requirement.

Manually we will be checking the entire flow of the application, including creating an account, signing in, requesting the token, approving it, etc.

== Challenges ==

There are a couple of challenges we have identified:

1. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

2. Making sure the database is secure: Since our database is going to store the encryption key and IV, it is of utmost importance that the database is secure and not prone to attacks.

== Future Scope ==

In future we would like to use multiple keys for encryption rather than just one constant key for a lifetime. These multiple keys will have a time to live. This will make the system more secure as now they will be different keys used at different times, so in case a key is leaked or even if it isn't, it won't last for much time, and soon a new key will be in place, bringing the system back to its initial secured position.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-15T02:22:28Z

Dgupta10: /* Class diagram for database in SSO */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The Problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:peerlogic.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:class_diagram.jpg]]

== Test plan ==

We will be testing our SSO app both manually and using automation using rspec.

Rspec tests will include:
1. Request a key, application should redirect to the main page with another listing added, with status as requested.
2. Admin approves a key, application should redirect to the main page with listing's status changed to approved and key generated and shown.
3. Admin declines a key, application should redirect to the main page with listing's status changed to declined.
4. User revokes a key, application should redirect to the main page with listing's status changed to revoked.
5. Clients send request, token is generated.
6. Client sends request with valid token, status sent 200 OK.
7. Client sends request with invalid token, status sent 401 Unauthorized.

There might be more tests added during the course of project development based on the requirement.

Manually we will be checking the entire flow of the application, including creating an account, signing in, requesting the token, approving it, etc.

== Challenges ==

There are a couple of challenges we have identified:

1. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

2. Making sure the database is secure: Since our database is going to store the encryption key and IV, it is of utmost importance that the database is secure and not prone to attacks.

== Future Scope ==

In future we would like to use multiple keys for encryption rather than just one constant key for a lifetime. These multiple keys will have a time to live. This will make the system more secure as now they will be different keys used at different times, so in case a key is leaked or even if it isn't, it won't last for much time, and soon a new key will be in place, bringing the system back to its initial secured position.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-15T01:41:52Z

Dgupta10: /* The problem */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The Problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:peerlogic.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:class_diagram.jpg]]

== Challenges ==

There are a couple of challenges we have identified:

1. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

2. Making sure the database is secure: Since our database is going to store the encryption key and IV, it is of utmost importance that the database is secure and not prone to attacks.

== Future Scope ==

In future we would like to use multiple keys for encryption rather than just one constant key for a lifetime. These multiple keys will have a time to live. This will make the system more secure as now they will be different keys used at different times, so in case a key is leaked or even if it isn't, it won't last for much time, and soon a new key will be in place, bringing the system back to its initial secured position.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-14T13:17:02Z

Dgupta10: /* Challenges */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:peerlogic.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:class_diagram.jpg]]

== Challenges ==

There are a couple of challenges we have identified:

1. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

2. Making sure the database is secure: Since our database is going to store the encryption key and IV, it is of utmost importance that the database is secure and not prone to attacks.

== Future Scope ==

In future we would like to use multiple keys for encryption rather than just one constant key for a lifetime. These multiple keys will have a time to live. This will make the system more secure as now they will be different keys used at different times, so in case a key is leaked or even if it isn't, it won't last for much time, and soon a new key will be in place, bringing the system back to its initial secured position.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-14T13:12:35Z

Dgupta10: /* Diving deeper into the SSO */

== Problem Statement ==
Description of the problem statement can be viewed on this [https://docs.google.com/document/d/1WYiXxhYyycp9a3I0GTC-4KFgHrg65Tf27ijJNQvxhmk/edit#heading=h.smsn92ion1nv link].

== PeerLogic ==

To understand the problem statement, let us first understand the terms used in the problem statement:

1. Peerlogic: Peerlogic is a service provider for education peer review systems. The project exposes a number of microservices or web APIs that can be consumed by peer assessment systems. It provides services for reputation algorithms, metareview metrics, etc. To give an example, a peer review system like Expertiza leverages Peerlogic APIs to assess whether a given review was competent enough or not. There are a dozen of such systems that leverage Peerlogic’s APIs for a number of purposes. More details on Peerlogic can be viewed [https://www.peerlogic.org/ here].

2. Expertiza: Expertiza is a peer assessment system that is build to help students review each others work and provide feedback, facilitating learning. Expertiza uses Peerlogic’s services for various purposes.

3. Single Sign On Portal: Single sign on portal is the system that E1795 aims at building. This will serve as an authentication layer between Peerlogic and all other applications using its API.

== The problem ==

Currently Peerlogic’s services can be used by anyone calling them. There is no system to check if an application accessing the services is actually allowed to do so. It is essential to have a system in place to authorize applications to avoid a number of security breaches that could occur using these open-to-all services and prevent them from being hacked.

== Proposed Solution ==

The proposed solution is to create a “Single Sign On Application” that will be responsible for providing authorization and authentication for the Peerlogic APIs. This application have two major modules:

1. User facing portal:
The user facing portal will be a deployed application with an user interface that a user/admin can use to register himself, request an API key for his/her app (in case of user), approve API keys (in case of admin), manage keys, and other similar functions.

2. REST APIs for Peerlogic:
There will be REST API created which Peerlogic can call to check whether a request received by Peerlogic is authorized and authenticated. This API is a core feature because it is responsible for actually putting the security system in place. If the API fails to return the correct result, an application not authorized to access Peerlogic may do so, leading to a security breach.

== Solution Architecture ==

In order to understand how the whole system will work, have a look at the figure given below.
[[File:peerlogic.jpg]]

The system includes a SSO portal, Peerlogic and different apps trying to access Peerlogic services. The types of users are admin and generic user (who will be responsible for creating a client account with SSO.

Note: In the following description, client has been used interchangeably with app.

1. Once the client users registers himself/the client, and client account is created. He can now request a client ID and a client key to be used by his/her app. (Step 1)

2. SSO sends this request forward to the admin, who either approve/reject this request. The admin tells whether the request is to be approved or not and forwards this information back to peerlogic. (Steps 2 and 3)

3. If the admin, approves the request, SSO generates a client ID and a client token that will be used by the client to send requests to SSO. (Step 4)

4. This client ID and key is seen by the consumer and they add this to the client app. (Steps 5 and 6)

5. When the client app decides it wants to call Peerlogic web services, it communicates with SSO, sending a request for a token, this request includes the client id and client key in order to let know SSO know he is a valid requester for token. (Step 7)

6. SSO generates a unique, one time use token for the client and passes it back to him. (Step 8)

7. The client now calls the Peerlogic web service and adds this token in the request as a header. (Step 9)

8. Peerlogic, after receiving the client request, sends a request to SSO with the token, asking if the token is valid and is allowed access to the service that client is requesting. SSO, at this point, checks does authorization and authentication. If all okay, it returns so. (Steps 10 and 11)

9. If Peerlogic receives a positive response from SSO, it lets client use its API, else returns an unauthorized/unauthenticated access error. (Step 12)

== Diving deeper into the SSO ==

Looking more deeply at the SSO, here are few key points that specify how we are going to implement the SSO:

1. We will use both google sign in and our own sign in to allow a user to create an account and log in.

2. We will use a random generator to generate the key for every client once the admin approves the request for the key. As a proposed solution we will use SecureRandom for this purpose, which is an algorithm to generate a random string. Although it is not optimized for uniqueness, if we are generating a 32 bit key, collision is very less likely to happen.

3. AES encryption algorithm, one of the safest encryption algorithms, will be used to generate the token for every client. This token will hold information including the client key, access rights and the time the token was generated.

4. When we decrypt the key, we will check whether we have such a client key in our database and whether they are allowed access to whatever it is that they are requesting. (This constitutes the authentication and the authorization portion)

5. We shall have a RoR application to handle all of this, and a database (whose model is discussed below).

== Class diagram for database in SSO ==

The following diagram shows the database that will be in work for the SSO application.
* API will store information about the different APIs and has two attributes: API_id and and the API name.
* Clients will store information about the clients. It will store which client id has what key. Also the owner of the client will be stored in this table.
* Access Rights will store information about which client has access to what APIs. This table will be used when we are checking the authorization of a particular client.
* Users will store information about the users/ The term user here is used to represent the entity which will represent the client. So a user account is nothing but a client facing account.
* Keys will store the value of the keys used and generated for encryption. Since we are using AES encryption algorithm, we need two values IV and Key. This values will be stored here along with the timestamp when they were generated. Why we have incorporated timestamp is because in future we might like to implement a TTL (Time to live) concept in the generation of token, in which case we can also use different keys for different times.

[[File:class_diagram.jpg]]

== Challenges ==

There are a couple of challenges we have identified that will be the key points we shall be working on:

1. Generation of token and its authorization/authentication: The process of unique token generation and its verification is the key abstraction. We shall be looking and analysing algorithms that can help us achieve this.

2. Creating a secure database that will be used by SSO: Creating a secure and normalised database to store all the information that SSO shall be using is another challenge.

3. Creating a client id and key and storing it: Given the rise of open source software, it is essential that the client id and key that is generated for an app is stored in a secure place, invisible to the world. We will be looking into ways how open source apps do this and good practices to do the same.

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-14T13:08:06Z

Dgupta10: /* Diving deeper into the SSO */

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-14T13:07:51Z

Dgupta10:

CSC/ECE 517 Fall 2017/E1795 Single signon for Peerlogic web services (WS intergration)

2017-11-14T13:01:15Z

Dgupta10: /* Class diagram */

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:12:09Z

Dgupta10: /* References */

=='''Introduction'''==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

=='''About response_controller.rb'''==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

=='''Tasks accomplished'''==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

=='''Refactoring Explained'''==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

==='''action_allowed? method'''===

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

==='''Replaced find_by_map_id with find_by'''===

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

==='''Replacing sorting technique'''===

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

==='''Refactoring pending_surveys method'''===

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

=='''References'''==
1. Expertiza Main Repo [https://github.com/expertiza/expertiza]

2. Expertiza Documentation [http://wiki.expertiza.ncsu.edu/index.php/Main_Page]

3. Pull Request [https://github.com/expertiza/expertiza/pull/1025]

4. Screencast for E1745 [https://www.youtube.com/watch?v=VfiPCQmnuAc]

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:11:56Z

Dgupta10:

=='''Introduction'''==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

=='''About response_controller.rb'''==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

=='''Tasks accomplished'''==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

=='''Refactoring Explained'''==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

==='''action_allowed? method'''===

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

==='''Replaced find_by_map_id with find_by'''===

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

==='''Replacing sorting technique'''===

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

==='''Refactoring pending_surveys method'''===

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

=='''References'''==
1. Expertiza Main Repo [https://github.com/expertiza/expertiza]
2. Expertiza Documentation [http://wiki.expertiza.ncsu.edu/index.php/Main_Page]
3. Pull Request [https://github.com/expertiza/expertiza/pull/1025]
4. Screencast for E1745 [https://www.youtube.com/watch?v=VfiPCQmnuAc]

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:07:50Z

Dgupta10: /* Refactoring Explained */

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:07:21Z

Dgupta10: /* Replaced find_by_map_id with find_by */

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:07:08Z

Dgupta10: /* Refactoring Explained */

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:05:31Z

Dgupta10: /* Introduction */

=='''Introduction'''==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

=='''About response_controller.rb'''==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

=='''Tasks accomplished'''==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

=='''Refactoring Explained'''==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:05:20Z

Dgupta10: /* About response_controller.rb */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

=='''About response_controller.rb'''==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

=='''Tasks accomplished'''==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

=='''Refactoring Explained'''==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:05:10Z

Dgupta10: /* Tasks accomplished */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

=='''Tasks accomplished'''==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

=='''Refactoring Explained'''==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:04:59Z

Dgupta10: /* Refactoring Explained */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

=='''Refactoring Explained'''==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:04:44Z

Dgupta10: /* Test Plan and Execution */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==Refactoring Explained==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

=='''Test Plan and Execution'''==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:04:31Z

Dgupta10: /* What all was tested */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==Refactoring Explained==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==Test Plan and Execution==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:04:09Z

Dgupta10: /* What all was tested */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==Refactoring Explained==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==Test Plan and Execution==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

=What all was tested=
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:03:52Z

Dgupta10: /* What all was tested */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==Refactoring Explained==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==Test Plan and Execution==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:03:40Z

Dgupta10: /* What all was tested */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==Refactoring Explained==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==Test Plan and Execution==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===''What all was tested''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:03:22Z

Dgupta10: /* Introduction */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==About response_controller.rb==
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==Tasks accomplished==
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==Refactoring Explained==
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==Test Plan and Execution==
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='''What all was tested'''===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='''Stubs and mocks'''===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:02:04Z

Dgupta10: /* Tasks accomplished */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included:
* Writing stubs and mocks
* Writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

===Stubs and mocks===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:01:04Z

Dgupta10: /* Test Plan and Execution */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

===Stubs and mocks===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-03T02:00:35Z

Dgupta10: /* Stubs and mocks */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

===Stubs and mocks===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

First let us understand how a mock object is created. Have a look at the following piece of code:

<pre> let(:review_response) { build(:response, id: 1, map_id: 1) } </pre>

This will build a response object (whose specifications are provided in the factories.rb file) with an id 1 and make it available through out the test process. We are therefore using this object as the object returned from a database without actually interacting with the database, thus "mocking" an object.

To understand how stubs work, consider the following piece of code in before(:each) block of the testing controller:

<pre>
allow(Response).to receive(:find).and_return(review_response)
</pre>

What this stub does is that when Response.find is called, instead of going into the database and finding an object and returning it, Rails will blindly return review_response mock. So this line tells Rails that if find is called on Response, return the pre created about review_response. This reduces the dependency on the database.

We wrote stubs and mocks required to successfully test all the pieces of the controller. All of it can be viewed in response_controller_spec.rb.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-02T19:31:48Z

Dgupta10: /* 'Stubs and mocks' */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc and filled out the preexisting test cases present, which were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are either an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

===Stubs and mocks===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-02T19:31:38Z

Dgupta10: /* Test Plan and Execution */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc and filled out the preexisting test cases present, which were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are either an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='Stubs and mocks'===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-02T19:31:27Z

Dgupta10: /* Test Plan and Execution */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
<pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc and filled out the preexisting test cases present, which were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are either an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='Stubs and mocks'===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-02T19:31:04Z

Dgupta10: /* 'What all was tested' */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
<pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting <pre> params = {action: "view", id: review_response.id} </pre>.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc and filled out the preexisting test cases present, which were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are either an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='Stubs and mocks'===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-02T19:30:51Z

Dgupta10: /* Integration Tests */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Test Plan and Execution'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
<pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting <pre> params = {action: "view", id: review_response.id} </pre>.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

==='What all was tested'===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc and filled out the preexisting test cases present, which were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are either an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

==='Stubs and mocks'===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-27T22:06:56Z

Dgupta10: /* Integration Tests */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Integration Tests'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are the stubs and mocks written for the testing:
<pre>
success_response = Net::HTTPResponse.new(1.0, 200, "OK")
current_round = 1
stage = nil
let(:assignment) { build(:assignment, instructor_id: 6) }
let(:instructor) { build(:instructor, id: 6) }
let(:participant) { build(:participant, id: 1, user_id: 6, assignment: assignment) }
let(:review_response) { build(:response, id: 1, map_id: 1) }
let(:review_response_map) { build(:review_response_map, id: 1, reviewer: participant) }
let(:questionnaire) { build(:questionnaire, id: 1, questions: [question]) }
let(:question) { Criterion.new(id: 1, weight: 2, break_before: true) }
let(:assignment_questionnaire) { build(:assignment_questionnaire) }
let(:answer) { double('Answer') }
let(:assignment_due_date) { build(:assignment_due_date) }

before(:each) do
stub_current_user(instructor, instructor.role.name, instructor.role)
allow(Assignment).to receive(:find).with('1').and_return(assignment)
allow(Assignment).to receive(:find).and_return(assignment)
allow(Response).to receive(:find).with('1').and_return(review_response)
allow(Response).to receive(:find).and_return(review_response)
allow(Response).to receive(:find_by).and_return(review_response)
allow(ResponseMap).to receive(:find).with('1').and_return(review_response_map)
allow(ResponseMap).to receive(:find).with(1).and_return(review_response_map)
allow(Participant).to receive(:find).with(1).and_return(participant)
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(assignment).to receive(:number_of_current_round).and_return(current_round)
allow(assignment).to receive(:get_current_stage).and_return(stage)
allow(review_response).to receive(:delete).and_return(success_response)
allow(review_response).to receive(:map).and_return(review_response_map)
allow(review_response).to receive(:questionnaire_by_answer).and_return(questionnaire)
allow(review_response_map).to receive(:assignment).and_return(assignment)
allow(review_response_map).to receive(:save).and_return(true)
allow(review_response_map).to receive(:questionnaire).with(current_round).and_return(questionnaire)
request.env['HTTP_REFERER'] = 'www.google.com'
end
</pre>

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-27T22:06:41Z

Dgupta10: /* Integration Tests */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Integration Tests'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are the stubs and mocks written for the testing:
<pre>
success_response = Net::HTTPResponse.new(1.0, 200, "OK")
current_round = 1
stage = nil
let(:assignment) { build(:assignment, instructor_id: 6) }
let(:instructor) { build(:instructor, id: 6) }
let(:participant) { build(:participant, id: 1, user_id: 6, assignment: assignment) }
let(:review_response) { build(:response, id: 1, map_id: 1) }
let(:review_response_map) { build(:review_response_map, id: 1, reviewer: participant) }
let(:questionnaire) { build(:questionnaire, id: 1, questions: [question]) }
let(:question) { Criterion.new(id: 1, weight: 2, break_before: true) }
let(:assignment_questionnaire) { build(:assignment_questionnaire) }
let(:answer) { double('Answer') }
let(:assignment_due_date) { build(:assignment_due_date) }

before(:each) do
stub_current_user(instructor, instructor.role.name, instructor.role)
allow(Assignment).to receive(:find).with('1').and_return(assignment)
allow(Assignment).to receive(:find).and_return(assignment)
allow(Response).to receive(:find).with('1').and_return(review_response)
allow(Response).to receive(:find).and_return(review_response)
allow(Response).to receive(:find_by).and_return(review_response)
allow(ResponseMap).to receive(:find).with('1').and_return(review_response_map)
allow(ResponseMap).to receive(:find).with(1).and_return(review_response_map)
allow(Participant).to receive(:find).with(1).and_return(participant)
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(assignment).to receive(:number_of_current_round).and_return(current_round)
allow(assignment).to receive(:get_current_stage).and_return(stage)
allow(review_response).to receive(:delete).and_return(success_response)
allow(review_response).to receive(:map).and_return(review_response_map)
allow(review_response).to receive(:questionnaire_by_answer).and_return(questionnaire)
allow(review_response_map).to receive(:assignment).and_return(assignment)
allow(review_response_map).to receive(:save).and_return(true)
allow(review_response_map).to receive(:questionnaire).with(current_round).and_return(questionnaire)
request.env['HTTP_REFERER'] = 'www.google.com'
end
</pre>

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:46:23Z

Dgupta10: /* Integration Tests */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

==='''Integration Tests'''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:45:56Z

Dgupta10:

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Pull request'''===

The pull request for this task can be viewed at [https://github.com/expertiza/expertiza/pull/1025]

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

===''Integration Tests''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:42:38Z

Dgupta10: /* Tasks done by the team */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks accomplished'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

===''Integration Tests''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:42:19Z

Dgupta10: /* Team Members */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks done by the team'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

===''Integration Tests''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:41:53Z

Dgupta10: /* 'Integration Tests */

==Introduction==
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

==='''About response_controller.rb'''===
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.

==='''Tasks done by the team'''===
1. Refactoring response_controller.rb

Included:
* Breaking down large methods into smaller chunks of readable reusable code.
* Changing code to adhere to latest Ruby conventions
* Creating functions to reuse chunks of code that were previously written multiple times

2. Testing response_controller.rb

Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.

==='''Team Members'''===
1) Pavneet Singh Anand

2) Dipansha Gupta

3) Kshittiz Kumar

==='''Refactoring Explained'''===
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-
<pre>

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end

def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end

</pre>

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end

# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32.
The refactored method is given below:-

<pre>

def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end

# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end

</pre>

===''Integration Tests''===
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality.
Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.

The following are a few test cases written by the team:

<pre>
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
</pre>

<pre>
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end

context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end
</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:36:54Z

Dgupta10: /* Tasks done by the team */

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-26T03:36:28Z

Dgupta10:

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-22T00:18:40Z

Dgupta10: Created page with "Testing"

Testing