Scribble V2

[yedhink]

https://public.3.basecamp.com/p/faivRsFjZyP6iwR6jmG11MKo

Lemme know if you have any queries.

@sunnyshahabuddin

[sunnyshahabuddin]

Added task Scribble V2 in my neetoPlanner.

[sunnyshahabuddin]

@yedhink Good morning, I have a doubt regarding Scribble V2:

• For the analytics part where we are showing the number of visits of an article, for that do I need to use any particular gem?

Also can you please guide me, on what is the best way to learn about a new gem and its usage. As when you asked to use paper trail in Scribble V2 docs. I went to the link attached there and read the official documentation but it was not very clear to me in the first place, then I had to read some articles regarding that gem. I just wanted to know like how you approach when you have to use a gem which you have not used before.

[sunnyshahabuddin]

@yedhink Good morning, Can you please confirm that what I understood from reading this is what you expect us to do, this is how my routes.rb looks after namespacing.

Also I have segregated the apis, test, controllers, views and services folder as well into the structure

|- api |-- admin |-- public

Is there something that I missed or is wrong, can you please point it out ?

Thank you.

[yedhink]

Can you please confirm that what I understood from reading this is what you expect us to do, this is how my routes.rb looks after namespacing.

This is correct.

Also I have segregated the apis, test, controllers, views and services folder as well into the structure

Also correct.

[sunnyshahabuddin]

@yedhink Good evening, I have few doubts which I have asked you in this video. Please help me with them. Sorry I missed one point in the video, since the version history is managed by paper_trail do I also need write test for that ?

https://www.loom.com/share/01e8c62b1a524dcd80fcf96ba036f183

[yedhink]

since the version history is managed by paper_trail do I also need write test for that

No need to test logic from paper_trail.

issue with getCurrentTab

Hooks should always be at the top of the component. If you are utilising hooks within a function, then that function also ought to be a hook. Here you don't have to create a custom hook. Rather do something like so:

const {search} = useLocation();
const currentTab = new URLSearchParamsBlahBlah(search);

// do things with currentTab

hooks folder

Custom hooks should be placed in src/hooks folder.

eslint error if use keyword is not used

use prefix suggests that it's a hook. https://reactjs.org/docs/hooks-rules.html

Using value prop in Formik

Keep it like so. But ideally we should not be passing value. Will look into that later when you raise the PR. Just remind me at that time. PS: Validation schema should be moved constants.js file.

Lemme know if all queries have been cleared or not

[sunnyshahabuddin]

Keep it like so. But ideally we should not be passing value. Will look into that later when you raise the PR. Just remind me at that time. PS: Validation schema should be moved constants.js file.

@yedhink Can I remind you the time when I submit my codebase for review ? I already merged that PR few days back, because that time I felt it was right, but going through the general feedback videos again today made me realise that should not be there. Sorry I should have asked you at the time of making the PR.

I will move all the validation schema into constants.js file.

Lemme know if all queries have been cleared or not

Thank you Yedhin. That cleared all my doubts.

[yedhink]

Can I remind you the time when I submit my codebase for review ? I already merged that PR few days back,

In that case don't worry about it. When you submit V2 for review, I will look into it.

[sunnyshahabuddin]

Thanks Yedhin.

[sunnyshahabuddin]

@yedhink I have completed Scribble V2, also tried to include the General Feedbacks changes. Please review.

Here is the link of my deployed application : https://scribble-by-sunny-shahabuddin.herokuapp.com/

Also you asked me remind you about the value prop in formik

[sunnyshahabuddin]

@yedhink I apologize for the four-hour delay in submission of my codebase for review, my deadline was 04/11/2022 and I submitted my codebase for review at 4am 05/11/2022. I thought it would be appropriate to inform you this explicitly.

Also should I mark Scribble V2 in my neetoPlanner as complete ?

[yedhink]

@sunnyshahabuddin

Go through V2 feedbacks: https://public.3.basecamp.com/p/wKBxu2BhTuAzaHTwU4ehnBd9

First estimate how much time you require for completing each of the tasks and then provide a date and time at which you will be resubmitting the app for review. Repeating the keyword time. I want time along with the date!

Tentative submission date: Monday before 11am. Update planner and put up a screenshot. If you need more time, then feel free to lemme know.

Also you asked me remind you about the https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/issues/208#issuecomment-1303761285

Will look into that later.

[sunnyshahabuddin]

@yedhink Good evening,

The changes that I need to make are:

• [x] Revamp the UI for version history.
• [x] Fix time zone issue in version history UI.
• [x] Add logic to invoke search in EUI with cmd+k, also show this on the UI.
• [x] Improve UI for analytics page.
• [x] Make test coverage to be 97%, with all models, controller and services test coverage to be 100%.

I have few doubts:

• I am not able to think of a logic of how can I show article visits date wise.
• Also the pagination provided by neetoUI Table is working, so do I need to use Kaminari for handling the paginations.

Except for the above two doubts, I will complete the rest before Monday 11am. Please help me clear my doubts.

Also in this case with what date should I update my neetoPlanner ?

[yedhink]

I am not able to think of a logic of how can I show article visits date wise.

You will need a separate table for maintaining visits.

Also the pagination provided by neetoUI Table is working, so do I need to use Kaminari for handling the paginations.

Kaminari is for handling the backend part of pagination. neetoUI table only takes care of the front-end logic regarding pagaination. Pagination is important to reduce the server load: https://stackoverflow.com/a/5346858

I will complete the rest before Monday 11am Also in this case with what date should I update my neetoPlanner ?

If you are confident on this date, then go ahead and update the same in your planner.

[sunnyshahabuddin]

@yedhink Good morning, I would be needing more time, because I will have to change my old logic completely in order to need to create a new table for maintaining the visits of articles, and learn kaminari gem to implement it in my codebase. Thereby I request you to kindly allow me to submit the codebase on tuesday 7pm and update the planner with the same.

[sunnyshahabuddin]

@yedhink

Extended by 3 days as per message in Slack. Here is the updated screenshot of my neetoPlanner. I will submit the codebase before 7pm 11/11/2022

[sunnyshahabuddin]

@yedhink Good evening, Screenshot of the test coverage

Link to my deployed application: https://scribble-by-sunny-shahabuddin.herokuapp.com/ Please review.

[yedhink]

@sunnyshahabuddin Fix these with high priority.

• [x] No need for this method invocation here: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/application_controller.rb#L14

• [x] Use the current_organization method here rather than using the memoized instance variable: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/application_controller.rb#L15

• [x] Let's split out the statements. Like invoke the service first. Get back the filtered articles. Then page in a separate statement. That's more cleaner. Also, no need for the render statement. It's implicit. https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/api/admin/articles_controller.rb#L7-L10

• [x] If the controller action is named total_count, then we ought to have a statement that gets the total count within the method. Here we are only getting the articles, which is almost similar to the index action. Also, try using counter_cache to get the total count of an entity: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/api/admin/articles_controller.rb#L43

• [x] Useless article variable declaration: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/api/admin/articles_controller.rb#L14

• [x] If you are using acts_as_list, then remove this logic and stick onto the methods provided by acts_as_list: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/api/admin/categories_controller.rb#L22-L29

• [x] Go through the Real world Rails quiz once again. We had already pointed out that setting the column name same as the table name is an anti-pattern. Also, use counter_cache to store the count: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/controllers/api/public/articles_controller.rb#L11

• [x] This absolutely wrong. Go through LRRB once again and point out to me why this statement is not right: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/models/article.rb#L16

• [x] If you are using acts_as_list, then I don't want to see any such custom logic pertaining to position. Stick onto the methods provided by acts_as_list: https://github.com/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/models/category.rb#L14-L17

• [x] I saw more than one person making the same mistake with the exact same code. Thus adding the same review comment I had given to the other person: 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/sunnyshahabuddin/scribble-by-sunny-shahabuddin/blob/68ece84680d63255282fbcf9c7064a7a7d14c49d/app/models/redirection.rb#L19-L21

[sunnyshahabuddin]

Sure, Yedhin working on the review comments right now.

[yedhink]

@sunnyshahabuddin Take a look into https://github.com/bigbinary/bigbinary-website/issues/2566 and make necessary change in your codebase. Also, I think you are using integer type for id column. If yes, then make sure to change it to uuid.

[sunnyshahabuddin]

@yedhink Yes I am using integer id, I once tried to use UUID but then it had some complication with paper_trail, I will it once again try to use UUID and make it work with paper_trail. If I face any issues I will ask your help for the same.

[sunnyshahabuddin]

@yedhink

Can you please confirm whether or not I understood why before_update callback was wrong in article.rb, also please point it out if I am wrong or missed out something. I also need your help in counter_cache, if you could provide me with some resource from which I can learn the implementation of it. Here is the video regarding the same.

https://www.loom.com/share/4c653c1cd66c4040be2c573581ea021f

[yedhink]

@sunnyshahabuddin

Can you please confirm whether or not I understood why before_update callback

What you've shown in your video is the correct way of using before_update. Now it's sensible.

learn counter cache

Refer:

• https://api.rubyonrails.org/classes/ActiveRecord/CounterCache/ClassMethods.html
• https://www.youtube.com/watch?v=goaKFUviwf4

[sunnyshahabuddin]

Thank you Yedhin.

[sunnyshahabuddin]

@yedhink I have I implemented counter_cache in my application, can you please point it out to me whether what I did is right or not ? I have explained what I did in the below videos.

https://www.loom.com/share/aa5c87515cdd416b8d377769c314f67d https://www.loom.com/share/e544e955886e4a28914e6edd7221be9a

Thank you.

[yedhink]

@sunnyshahabuddin For handling counter caches conditionally, you'd have to use something like counter_culture gem.

We do use this gem in BigBinary.

Refer:

• You can set multiple count columns and certain columns are conditional counts: https://github.com/magnusvk/counter_culture#conditional-counter-cache

[sunnyshahabuddin]

I will try to implement this, and get back to you if I face any blockers.

[sunnyshahabuddin]

@yedhink I have used counter_culture gem for displaying the articles count conditionally. Here is the link to my deployed application- https://scribble-by-sunny-shahabuddin.herokuapp.com/

Please review.

[yedhink]

@sunnyshahabuddin https://www.loom.com/share/058147f4ced245aca0cd89d137d33ad3

[sunnyshahabuddin]

@yedhink

@sunnyshahabuddin https://www.loom.com/share/058147f4ced245aca0cd89d137d33ad3

Good afternoon, I will fix this and submit this at the time of submission of V3, will that be fine ?

[yedhink]

I will fix this and submit this at the time of submission of V3, will that be fine ?

Yes. That's fine.

[sunnyshahabuddin]

Thank you.