This wiki page describes the changes are made in the Expertiza source code to implement E1920 OSS Project, "Fix Code Climate issues in models with names beginning with 'Se' thru 'Z'" in the Spring 2019 semester, CSC/ECE 517.

== Introduction ==

This is intro section. We can talk about what is our project and how we got started.

=== Expertiza ===

Briefly talk about Expertiza.

=== Code Climate ===

Code Climate is an analysis software used to improve the quality and security of your code. The Code Climate results for the Expertiza project can be found [https://codeclimate.com/github/expertiza/expertiza/code here].

Code Climate will use various engines to analyze your code to look for issues that may concern potential security issues, (.i.e. marking code to html_safe), or add to maintainability issues (.i.e number of lines in method exceeding 25 lines). Some of the engines include: RuboCop, Brakeman, Bundler-Audit, [https://docs.codeclimate.com/docs/list-of-engines etc.]

Code Climate can ran from either a 3rd party remote server of via the local application using CLI. The Expertiza project uses the 3rd party remote server to analyze the code. In order to use CodeClimate via remote server, you have to register your repository. Following [https://docs.codeclimate.com/docs this] process will allow you to check your code for quality issues after Pushing your code to the monitored repository.

== Project Description ==

Some of the Expertiza source code violates many of the [https://rubyonrails.org/ Ruby on Rails] best practices and must be rectified to improve code readability, robustness and maintainability of the Expertiza application.

Our team was assigned to fix ''all'' code smells detected by [https://codeclimate.com/github/expertiza/expertiza/code Code Climate] in the given set of Expertiza source code files.

=== Files Involved ===

All files in the [https://github.com/expertiza/expertiza/tree/master/app/models app/models] directory with names beginning with '''Se''' (e.g. [https://github.com/expertiza/expertiza/blob/master/app/models/section_header.rb section_header.rb]) through '''Z''', except [https://github.com/expertiza/expertiza/blob/master/app/controllers/users_controller.rb users_controller.rb] file were targeted by our team to be Code Climate issue free files. Therefore, our team was assigned exactly with 50 (fifty) files to fix in total. Some of the files had no issues reported by the Code Climate to begin with, and hence were not modified. However, changes/improvements performed on other files had an affect globally on the files outside of the app/models scope and also had to be modified to adapt changes we implemented. For example, renaming method ''is_leaf'' as follows:
def leaf?
in the [https://github.com/expertiza/expertiza/blob/master/app/models/team_user_node.rb team_user_node.rb] file require to change all callers to this method in different files accordingly. Such changes had to be done in the following files (for this particular example):
<ul>
<li> models/assignment_node.rb
<li> models/node.rb
<li> models/questionnaire_node.rb
<li> views/tree_display/_row_header.html.erb
</ul>

These files are outside of the requirements specifications, but we had to modify them in order to maintain correct behavior of the App. There were few such cases when we had to modify multiple files to fix one or more Code Climate issue(s). The list of all of files changed can be found in the Pull Request discussed in this article.

=== Ignored Code Climate Smells ===

There are some Code Climate issues that were ignored and not resolved intentionally.

The following issues are exempt based the given requirements specifications and were not fixed:
<ul>
<li> Assignment Branch Condition size for [method name] is too high.
<li> Perceived complexity for [method name] is too high.
<li> Cyclomatic complexity for [method name] is too high.
<li> Method [method name] has a Cognitive Complexity of ''XX'' (exceeds 5 allowed). Consider refactoring.
<li> File [file name] has ''XXX'' lines of code (exceeds 250 allowed). Consider refactoring.
<li> Class [class name] has ''XX'' methods (exceeds 20 allowed). Consider refactoring.
<li> Method [method name] has ''XX'' lines of code (exceeds 25 allowed). Consider refactoring.
</ul>

There are also two Code Climate issues that our team decided to ignore after consulting our mentor and discussing their impact on the existing code and Expertiza project. It was determined that the following Code Climate issues reported as False Positive and may not need to be resolved:
<ul>
<li> Mass assignment is not restricted using attr_accessible.
<li> Potentially dangerous attribute available for mass assignment.
</ul>

Finally, there was a duplicate code issue that was ignored due to a future project. The following files have a duplicate method, self.export_fields(options):
<ul>
<li> user.rb
<li> participant.rb
</ul>

For further clarification with regard to ignored Code Climate issues please contact our mentor.

== Project Implementation ==

Talk about how we fixed the most common issues reported by Code Climate.

=== Most Common Issues Reported by Code Climate ===

As we worked on the Code Climate issues for the Expertiza project, we noticed a set of common code smells and issues made by software developers. Since these issues can easily be avoided and most importantly, avoiding them improves code quality, we decided to ''warn'' all Expertiza developers about them by providing the list of these issues, their descriptions, how they can be avoided, and how the code quality can be improved.

* Prefer ''each'' over ''for'':
We noticed there is a lot of iteration in the code using ''for'' loop instead of ''each''. Generally, you want to avoid any ''for'' loops and use ''each'', ''each_index'', ''each_with_index'', ''upto'' and etc. For example, (code sample is used from ''app/models/sign_up_sheet.rb'' file):
# Bad - use of for loop
for user_signup_topic in user_signup
return false if user_signup_topic.is_waitlisted == false

# Good - use of each
user_signup.each do |user_signup_topic|
return false if user_signup_topic.is_waitlisted == false
Similarly, if you need to iterate the given block, passing in integer as an index, utilize ''each_index'', ''each_with_index'', ''upto'' instead of ''for''. For example, (code sample is used from ''app/models/sign_up_sheet.rb'' file):
# Bad - use of for loop
for round in 1..@review_rounds
process_review_round(assignment_id, due_date, round, topic)

# Good - use of upto
1.upto(@review_rounds) do |r|
process_review_round(assignment_id, due_date, r, topic)
* Specify an '':inverse_of option'':
Whenever there are two models defined with many|one-to-one relationship (''has_many''|''has_one'' and ''belongs_to'' association) out of scope of each other or with certain options used (e.g. ''foreign_key:'' and etc.), Active Record is unable to determine the inverse association of them. This would cause redundant look up operation performed on the database for the same object. It is not really an issue that could cause significant problems, but for such massive application like Experiza, it could affect performance and cause delay in response. Therefore, the '':inverse_of'' option must be specified manually to use the associated object in memory instead of loading it from database twice. For example, (code sample is used from ''app/models/teams_user.rb'' and ''app/models/team_user_node.rb'' files respectively):
class TeamsUser < ActiveRecord::Base
has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy, inverse_of: 'node_object'
end

class TeamUserNode < Node
belongs_to :node_object, class_name: 'TeamsUser', inverse_of: :team_user_node
end
* Tagging a string as html safe may be a security risk:
For example,
html.html_safe # George can you please talk about it how you fixed it
* Use ''find_by'' instead of ''where.first'':
Always use ''find_by'' with an attempt to find the first record matching the specified conditions. However, keep in mind that ''find_by'' and ''where_first'' return different object IFF no record was found based on specified conditions.
<ul>
<ul>
<li> ''Model.find_by'' - if there is a record match, returns the object. If there is no record match, returns ''nil''
<li> ''Model.where.first'' - if there is a record match, returns first ActiveRecord::Relation matched. If there is no record match, returns an Empty ActiveRecord::Relation
</ul>
</ul>
Therefore, always handle ''nil'' appropriately if there is no record match (with ''if/else'' block and etc.) Do not assume that the record will always be found. For example,
user = User.find_by(name: row_hash[index.to_sym].to_s)
raise ImportError, "The user, " + row_hash[index.to_sym].to_s.strip + ", was not found." if user.nil?
* Use ''snake_case'' for method names and for variable names:
There are a lot of instances where developers use ''camelCase'' instead of ''snake_case'' in the Expertiza project naming methods and variables. Since Expertiza project is based on RoR, it is preferred to use ''snake_case'' for all method and variable names. For example, (code sample is used from ''app/models/sign_up_sheet.rb'' and ''app/models/sign_up_topic.rb'' files receptively):
# Bad method name - camelCase
def self.slotAvailable?(topic_id)

# Good method name - snake_case
def self.slot_available?(topic_id)

# Bad variable name - camelCase
dropDate = AssignmentDueDate.where(parent_id: assignment.id, deadline_type_id: '6').first

# Good method name - snake_case
drop_date = AssignmentDueDate.where(parent_id: assignment.id, deadline_type_id: '6').first

=== Resolved Code Climate Issues ===

Along with [http://wiki.expertiza.ncsu.edu/index.php/E1920_Fix_Code_Climate_Issues#Most_Common_Issues_Reported_by_Code_Climate Most Common Issues Reported by Code Climate] we also fixed and verified numerous other code smells. The following is the list of all issues we have resolved:

<ul>
<li> Avoid using ''update_attribute'' because it skips validations.
<li> Convert ''if'' nested inside ''else'' to ''elsif''.
<li> Do not prefix reader method names with ''get_''.
<li> Do not use ''DateTime.parse.strftime'' without zone. Use one of ''Time.zone.parse.strftime'', ''DateTime.current'', ''DateTime.parse.strftime.in_time_zone'', ''DateTime.parse.strftime.utc'', ''DateTime.parse.strftime.getlocal'', ''DateTime.parse.strftime.iso8601'', ''DateTime.parse.strftime.jisx0301'', ''DateTime.parse.strftime.rfc3339'', ''DateTime.parse.strftime.to_i'', ''DateTime.parse.strftime.to_f'' instead.
<li> Prefer Date or Time over DateTime.
<li> Rename ''has_x'' to ''x?''.
<li> Rename ''is_x'' to ''x?''.
<li> Specify an '':inverse_of option''.
<li> Unnecessary spacing detected.
<li> Use ''find_by'' instead of ''where.first''.
<li> Useless assignment to variable - ''x''.
</ul>

=== Additional Improvements ===

Talk about additional improvements if there are any ( I(Nikolay) had some some I will include them here).

=== Remaining Issues ===

Talk about remaining issues and further improvements.

=== Further Suggestions ===

Talk about suggestions you have on files that you fixed if there are any? May be break down some methods into several and etc?

=== Pull Request ===

Let's add some info about pull request

== Testing ==
There we 2 types of frame works that we used to verify our code modifications/creation within the models: RSpec and Travis CI

RSpec was used to locally test the changes we had implemented without having to Push the change to the GitHub, this is discussed more in-depth in the section below.

Travis CI is a more comprehensive testing used by the Expetiza team to verify code changes from multiple sources, (.i.e forks from students). Travis CI will analyze the code once you Push your changes to the GitHub server for your given repository. Travis CI will need to be setup independently for your repository

Talk about Testing we performed

=== Rspec Testing ===

To verify the code changes we made did not break the existing application, we used the RSpec Testing Framework. Several of the models already had existing RSpecs Tests already created. For those, we just simply ran the corresponding test via the terminal window. Example
rspec spec/models/tag_prompt_spec.rb

*rspec path/to/spec/file.rb*

This would execute the RSpec test for the tag_prompt.rb file located in the app/models folder. As you can see, there is a particular format that is used when naming the RSpec file:
model_name_spec.rb

Once the execution of the RSpec model testing is completed, results will be display
$ rspec spec/models/tag_prompt_spec.rb [10:45:15]
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's 'rails' settings.
Randomized with seed 19443
.........
Finished in 3.35 seconds (files took 23.23 seconds to load)
9 examples, 0 failures
Randomized with seed 19443
Coverage report generated for RSpec to /home/expertiza_developer/RubymineProjects/expertiza_testbed/coverage. 1254 / 17313 LOC (7.24%) covered.
[Coveralls] Outside the CI environment, not sending data.

In addition to running the individual RSpec Test, we also executed broader in scope testing. To achieve this, we executed the following command in the terminal window:
$ bundle exec rspec --seed 5114

This produces the following results:

Finished in 1 minute 11.23 seconds (files took 23.19 seconds to load)
131 examples, 1 failure, 1 pending

Failed examples:

rspec ./spec/features/assignment_creation_spec.rb:707 # assignment function check to find if the assignment can be added to a course

Randomized with seed 5114

Coverage report generated for RSpec to /home/expertiza_developer/RubymineProjects/expertiza_testbed/coverage. 2998 / 14031 LOC (21.37%) covered.
[Coveralls] Outside the CI environment, not sending data.
FAIL: 1

As you can see from the examples above there are failures. However, we are only looking for failures caused by the code we modified/created in a given model. In this case, all code we have created/modified is not causing failures.

=== UI Testing ===

We concentrated our UI testing based on how changes to Code Climate issues affected the interface between callers and methods. Two main culprits were found. The first was the change from Time.parse to Time.zone.parse. While Time.parse returns an error for invalid or unknown times, Time.zone.parse returns nil for those types. The second culprit was changing where().first to find_by(). Using where().first return empty records if nothing is found, while find_by() returns nil is nothing is found.

The user interface testing concentrated on three type of user stories. The first set of user stories was general functionality, could users still access the basic menu items from before. The second set of user stories were those that dealt with time issues, such as sorting. The third set of user stories were those that dealt with searching.

== Conclusion ==

Talk about conclusion.

== Team Contacts and Contribution ==

We have 3 (three) members in our team and 1 (one) mentor was assigned to us. For any project-related questions/concerns please contact us:

<ul>
<li> Christopher Adkins - cladkins@ncsu.edu
<li> George Hugh - gshugh@ncsu.edu
<li> Nikolay G. Titov - ngtitov@ncu.edu
<li> Zachariah Mathews (mentor) - zmathew@ncsu.edu
</ul>

If you have questions or concerns related to a specific file that was improved/modified by our team, please contact an engineer who worked on the file based on the [https://docs.google.com/spreadsheets/d/1bX2XizitGGoajFT6Br_y6MQjAwBCsX5WCVlx3xg8Jp8/edit?ts=5c87a06c#gid=0 Assignment Grid] we created to keep track of our progress and contribution. Please CC in email all team members and our mentor.

== References ==
1. https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/InverseOf<br>

E1920 Fix Code Climate Issues

2019-03-24T16:49:51Z

Cladkins: /* Testing */

E1920 Fix Code Climate Issues

2019-03-24T15:59:54Z

Cladkins: /* Rspec Testing */

E1920 Fix Code Climate Issues

2019-03-24T15:59:33Z

Cladkins: /* Rspec Testing */

E1920 Fix Code Climate Issues

2019-03-24T13:22:43Z

Cladkins: /* Rspec Testing */