Review scribble

[ravijyoti3]

@yedhink Please review. Here is the deployed link.

[yedhink]

@ravijyoti3

• [x] Opening a preview of an article from dashboard shows undefined as slug. That's not right.

  

• [x] Regarding the bulk update logic: So tell how you would write a rake task that would delete a specific category from production env and move the articles from within it to another category? How will you utilise this controller logic to do that and also ensuring things are DRY?

• [x] Settings. Plural!

  

Go through the latest feedbacks and submit the fixed app by EOD tomorrow. Update planner and put up a screenshot here.

[ravijyoti3]

@yedhink Hi, I am able to implement the bulk update logic using the rake task like shown below, but I am unable to figure out, how to do it by utilizing the controller logic.

task bulk_update: [:environment] do
  bulk_update
end

def bulk_update
  Article.where(category_id: 3).update_all(category_id: 4)
  Category.find(3).destroy
end

Can you please help me with this?

[yedhink]

but I am unable to figure out, how to do it by utilizing the controller logic.

Exactly. Thus move the logic to a service and invoke the service as part of the destroy action rather than creating separate actions itself. Point being stuffing all logic into the controller can be counter productive, and also can be very limiting, like with the rake task I had mentioned above.

[yedhink]

@ravijyoti3 Take time till tomorrow EOD to finish these things up.

[yedhink]

Also ensure coverage is atleast 90%. Refer LRRB on how to do test coverage.

[ravijyoti3]

Sure Yedhin, I will do that.

[yedhink]

Go through the latest general feedbacks I've put up in the basecamp doc.

• [x] Ensure test coverage is atleast 90%. Refer LRRB to understand what test coverage is. Put up a screenshot once the test coverage is above 90%.
• [x] Ensure your circleci test suite is green. That's all test cases are passing.

Make the necessary changes and submit the app on October 31st morning before 11am. Update planner and put up a screenshot.

[ravijyoti3]

@yedhink please review.

Test coverage: https://monosnap.com/file/AKUpc8jjf3GTIe1ESNsMXxJ74L0BDo

NeetoPlanner screenshot: https://monosnap.com/direct/1m9ldO5OCRVqNkOfhSNmHFNmwWgtJ9

Deployed at https://scribble-by-ravi-jyoti.herokuapp.com/

[yedhink]

@ravijyoti3 Please go through the latest general feedbacks and update your repo.

[ravijyoti3]

@yedhink please review. I have updated the code based on the latest general feedback.

[yedhink]

@ravijyoti3 https://www.loom.com/share/22851f007eba4df3add3c578f10ce860

[ravijyoti3]

Sorry, Yedhin. I missed checking for the latest update on basecamp, I worked on the feedback till 31st October. I will make the required changes.

[yedhink]

@ravijyoti3 When exactly will you be submitting V1 for review? I need a date and time on submission from your end.

[ravijyoti3]

@yedhink Sorry for the delay. I have tried my best to ensure that all the changes are covered as per the general feedback. Please Review.

[yedhink]

@ravijyoti3 https://www.loom.com/share/52dcf33b599345c48611b2843cf00f77

[ravijyoti3]

@yedhink Sorry for this, I actually got confused that I only need to do the namespacing for admin & public, not for API. That was my mistake of assuming things rather than clarifying the doubt with you. I will submit Scribble V1 for review, on Monday at 11:00 am.

[yedhink]

@ravijyoti3 Update planner and put up a screenshot.

[ravijyoti3]

Yedhin, I have updated the planner, here is the screenshot.

https://monosnap.com/file/B2MxPcTMT9iSUU0evtReLNDXes71rw

[ravijyoti3]

@yedhink please review. Here is the deployed link https://scribble-by-ravi-jyoti.herokuapp.com/

[yedhink]

@ravijyoti3 Feedback: https://www.loom.com/share/4c1ee9d717ad4ef8a1715b7be43996c2

[ravijyoti3]

Thanks for the feedback Yedhin. I will make these changes in my code.

[ravijyoti3]

@yedhink Extended by 3 days as per message in Slack.

[ravijyoti3]

@yedhink Please review. I have made the changes as per the feedback.

[yedhink]

• [x] Why do you need this logic even after using acts_as_list? https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/controllers/api/admin/categories_controller.rb#L17-L27

• [x] Service should raise an exception on failure rather than using if-else: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/controllers/api/admin/categories_controller.rb#L39

• [x] Go through the Real world Rails quiz once and fix the issue with the following code: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/controllers/api/admin/categories_controller.rb#L12-L13

• [ ] This is so not right. At this stage of Scribble I should not be even pointing this out. Use update!: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/controllers/api/admin/organizations_controller.rb#L11-L14

• [x] Useless statement: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/controllers/application_controller.rb#L14

• [x] First of all why have you suffixed with a bang(!)? Second, why's this named load_organization over current_organization? If you have a strong reason to use load_organization, then why did you name the other method as current_user over load_user? https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/controllers/application_controller.rb#L9-L13

[yedhink]

• [x] Pointless logic. Why are you even using acts_as_list if you are writing all this logic by yourself? https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/models/category.rb#L15-L18

[yedhink]

• [x] This logic is not right. Example:

  1 -> 2
  2 -> 5
  3 -> 4
  5 -> 3

  ^ This is not cyclic. But your code will mark it as cyclic. https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/models/redirection.rb#L21

[yedhink]

• [ ] Use uuid in DB for id column rather than integers. Make this change immediately: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/services/article_filteration_service.rb#L14

[yedhink]

• [x] Use a partial because you have same logic in index: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/views/api/admin/categories/show.json.jbuilder#L3-L4

• [x] It's very stupid to send the password_digest to the front-end side. Why do you even need to do that? https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/cebf4b51d75191bef7f4dec83d019a90832025eb/app/views/api/admin/organizations/show.json.jbuilder#L3

[yedhink]

@ravijyoti3 Put up a date and time on which you will resubmit the app for review again. Update planner and put up a screenshot for the same.

[ravijyoti3]

@yedhink I will be submitting it on Monday at 5 PM.

[ravijyoti3]

@yedhink While updating the organization details, if I use update! then it is not updating the password_digest. But when I used the save method, password_digest was updated successfully.

[ravijyoti3]

@yedhink I have made the recommended changes, except for the this issue because I have used query params in Axios and I don't have to convert the id to integer explicitly. Do I still need to add UUID?

And also I am not able to use the update action to update organization details as mentioned above.

[yedhink]

@ravijyoti3

Do I still need to add UUID?

All I meant to say is that, whatever data you are storing in the database, should contain ID as uuid and not integer values. Make necessary changes in DB level, if you are using ID with integer type. Refer LRRB.

And also I am not able to use the update action to update organization details as mentioned https://github.com/ravijyoti3/scribble-by-ravi-jyoti/issues/106#issuecomment-1313405831.

In the following invocation you are not passing in the parent payload key, which is organization: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/04c63f37a4d26948a393e7886632f9030a744601/app/javascript/src/components/Eui/Login.jsx#L17-L19

Thus the parent key ought to be added via the API connector, which is in the following: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/04c63f37a4d26948a393e7886632f9030a744601/app/javascript/src/apis/admin/organization.js#L4

The biggest anit-pattern is that, you've NOT used strong parameters, which suggests to me that you haven't actually understood why we permit certain keys explicitly in the params. https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/04c63f37a4d26948a393e7886632f9030a744601/app/controllers/api/admin/organizations_controller.rb#L10-L16

if I use update! then it is not updating the password_digest. But when I used the save method, password_digest was updated successfully.

You've not understood what the xxx_params methods is doing. Example: Have you even understood what the following statement is doing and why we need to create such a method? https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/04c63f37a4d26948a393e7886632f9030a744601/app/controllers/api/admin/categories_controller.rb#L36-L38

So this is what I want you to do:

• [x] Go through LRRB and revise regarding what strong parameters are. Then make necessary changes in the controller level as well as in the front-end apis.
• [x] Put up a video and answer all the questions that i've raised here.

I need this done by tomorrow before 12PM sharp. No exceptions will be allowed.

[ravijyoti3]

@yedhink I have gone through the LRRB, but still I am not able to figure out the following issue. Please help me out with it. https://www.loom.com/share/f1f9c6e2cc0d4ff3b6733055049ed9a3

[yedhink]

@ravijyoti3 https://www.loom.com/share/251bf53020714aad9a371c1aff0cba10

[ravijyoti3]

@yedhink Sorry for misinterpreting the review and the delay caused by it. I have made the required changes for updating the organization details.

https://www.loom.com/share/a0e599ecceaf4bb0b32145b10d9ed42d

[yedhink]

@ravijyoti3

• [x] Go through LRRB very carefully and figure out what's wrong with the following statement: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/1ec924274e95d5f7f703a38545b404605fd58b51/app/models/article.rb#L14

• [x] Why do you need this statement? https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/1ec924274e95d5f7f703a38545b404605fd58b51/app/services/article_filteration_service.rb#L14

• [x] Move this out into well named private methods: https://github.com/ravijyoti3/scribble-by-ravi-jyoti/blob/1ec924274e95d5f7f703a38545b404605fd58b51/app/services/article_filteration_service.rb#L15-L17

[ravijyoti3]

@yedhink I went through LRRB again, and I found that before_save is not a good choice because the set slug function will be invoked every time the article is saved. before_create is a better choice, but since we have to set slug only when an article is published for the very first time. We can use before_update along with before_create.

I have made the above fixes in the latest PR.

Thank you.

[ravijyoti3]

@yedhink https://github.com/ravijyoti3/scribble-by-ravi-jyoti/issues/106#issuecomment-1317505663 Is this the right way to set slug?

[yedhink]

https://github.com/ravijyoti3/scribble-by-ravi-jyoti/issues/106#issuecomment-1317505663

This will work only if you add conditional invocation for each of these callbacks. Have you done that?

[ravijyoti3]

#106 (comment)

This will work only if you add conditional invocation for each of these callbacks. Have you done that?

Yes, I have added conditional invocation.