Track Progress on Strategies #1719
Conversation
|
@GayathriVenkatesh thanks for the PR! Would you mind providing a PR description of the general strategies you used? This is a pretty large PR, so contextual help from you as the author would be very appreciated. 🙇 |
| @@ -1,5 +0,0 @@ | |||
| class AddVisibleToStrategies < ActiveRecord::Migration[5.2] | |||
| def change | |||
| add_column :strategies, :visible, :boolean, default: true | |||
There was a problem hiding this comment.
Let's not modify this file. I would remove changes to this file from your PR. We shouldn't be modifying previous migration files in any way. This may be responsible for the test migration error in CircleCI.
There was a problem hiding this comment.
So you have to remove this change entirely from this pull request. This is still causing the test migration error in CircleCI. There are two ways you can do this:
-
You should do this by squashing all your commits into one. Doing a soft reset and remove this file and re-commit everything else.
-
Revert the specific commit that made this change
db/migrate/20200224201131_create_moment_categories_join_table.rb
Outdated
Show resolved
Hide resolved
GayathriVenkatesh
force-pushed
the
strategies
branch
from
ce60f85
to
1e088d4
Compare
GayathriVenkatesh
force-pushed
the
strategies
branch
from
01d5e92
to
0ec0bad
Compare
| @@ -94,12 +103,14 @@ def update | |||
| end | |||
| empty_array_for :viewers, :category | |||
| shared_update(@strategy, strategy_params) | |||
| create_task(@strategy) | |||
There was a problem hiding this comment.
We should be testing for this new task functionality in the test for update.
| end | ||
|
|
||
| # DELETE /strategies/1 | ||
| # DELETE /strategies/1.json | ||
| def destroy | ||
| shared_destroy(@strategy) | ||
| Task.where(id: @strategy.id).destroy_all |
There was a problem hiding this comment.
We should be testing for this new task functionality in the test for destroy.
| @@ -0,0 +1,23 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| module TasksHelper | |||
There was a problem hiding this comment.
We should be unit testing this helper.
app/controllers/tasks_controller.rb
Outdated
| @tasks = Task.all | ||
| end | ||
|
|
||
| def show; end |
There was a problem hiding this comment.
Do we actually need this route?
|
@julianguyen could you help with the circleci build error. It says lines 155 and 171 of |
|
@Manasa2850 It looks like you have two tests in the StrategiesHelper spec failing. You'll want to compare the the expected output and actual output to determine what went wrong. Your changes do affect these tests because you are adding |
Description
This PR creates a Task page that renders all your strategies and displays your progress in each one of them. I have added a column 'Mark as Finished' to the strategies table. I have also created a task strategies:update which automatically runs every 24 hours and updates no_of_days_followed for each strategy (based on its 'finished' value) and the total_no_of_days (date.today - date of creation).
The page looks like this so far :
My initial thought was to also leverage this data to render progress charts and help in easy visualisation of data. Hence, this PR is not complete. If you have any suggestions on what else the page could render and make it more usable, please leave your thoughts in the comments below!
Corresponding Issue
#1687
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!