[#26] [BackEnd] As a User, I can see a list of my previous keywords

[malparty]

• [BackEnd] As a User, I can see a list of my previous keywords #26

What happened

• Create Keyword model
• Add seeds
• Add keyword#index Model query to list keywords (include pagination)

Insight

• Create Keyword model
  • name has index
  • belongs to a user
• Add seeds
  • admin user (dev environment) in order to seed keywords to a user
  • 100x random keywords to this admin user
• Add keyword#index Model query to list keywords (include pagination)
  • using the Query Object pattern
    • only order by name asc (at the moment)
    • build to ease future extension
  • use pagy gem for pagination

Proof Of Work

Tests cover the keywords_query object:

UI tests will be added in the FrontEnd issue #27 and requests tests in the #25

[github-actions]

	3 Warnings
⚠️	app/models/user.rb#L16 - Remove unused methods (user#authenticate)
⚠️	app/models/user.rb#L18 - Is controlled by argument 'password'
⚠️	app/models/user.rb#L21 - Remove unused methods (user#create_access_token)

Generated by 🚫 Danger

[ankitkalia1195]

According to our compass conventions, should we use soft delete here.

   Always soft-delete relational data for integrity and audit purposes. Gems such as paranoia and discard provide ready-to-use solutions.

[malparty]

Yes, makes perfect sense.

As there will be no delete/destroy action exposed for keywords, I will try to handle it on the only scenario when a user "cancel his account" (so that it soft-delete the user, but it can keep keywords as is).

Would that be ok @ankitkalia1195 ? :) Thanks!

[malparty]

ok, soft delete for Users implemented and tested.
Do you think we need to add tests @ankitkalia1195 ?
Candidates for tests:

• Once deleted, a user get logged out
• Once deleted, a user cannot log in anymore
• Once deleted, a user cannot reset his password

it's build-in with devise and discard, so I'm not sure if tests are needed in here~
Thanks for your advises :)

[ankitkalia1195]

@malparty It seems you have again overcomplicated things, but its okay. For testing soft delete in models, you could have just tested them via rails console. We add soft delete while designing the model class, does not necessary mean that we should have some UI to test it.

[ankitkalia1195]

The main motive is to just ensure that data does not get deleted from database, whether is user or keyword. So usually when working on projects, we add soft delete to models straight away. Even though there might not be a way for users to delete it directly

[ankitkalia1195]

For reference, you can check project mia. https://github.com/nimblehq/mia-web/tree/development/app/models. Here we have acts_as_paranoid in all models

[malparty]

yes, the idea was to practice, there is a high chance that in a future project I get the need for a user soft-delete feature ^^,
For the choice of discard over paranoia, it's due to the notice heading paranoia readme:

paranoia has some surprising behaviour (like overriding ActiveRecord's delete and destroy) and is not recommended for new projects. See discard's README for more details.

I'll remember to add a soft-delete even when not needed -- with the intend to prevent unexpected hard-delete into future features.

[ankitkalia1195]

@malparty Sure, we can use discard as well, altough in most of our projects we still use paranoia.

[ankitkalia1195]

Basically paranoia is easier to use and does everything magically, discard is more explicit, and you have to write more code in order to achieve the result. In our rails template we still prefer paranoia, but you can bring it to a vote if it should be discard

[malparty]

Ok, yes I saw paranoia replace the destroy/delete methods so it has this "magic rails" feel :)
As I started with discard and as you said, I already complicated things, then I've just added the soft delete to keywords -- as per convention.

Some little rails console test:	It fills the discarded_at date:

And as the scope of keywords is kept, it does not display the keywords anymore:

[junan]

I think it should be private, as we are not accessing the keyword like KeywordsQuery.new(User.new).keywords

...
private

attr_reader :keywords
...

[malparty]

But then we do not need the attribute at all, no? :)
(I've added the attribute as it was part of an example of a QueryObject pattern -- but indeed, if we do not need it, there is no point to have it).
I've updated the code to remove the attr, let me know if it's ok @junan :)

[junan]

@malparty attr_reader actually generates a getter method behind the scene. When we read data, we dont directly call the instance variable(ex: @Keywords), rather we retrieve the data with a getter method (ex: keywords method which generated by attr_reader :keywords).

For example this class

class SomeClass
  # Define getter method in public if we want to access a, b, c outside of the class
  attr_reader :a, :b, :c
  
  def initialize(a, b, c)
    @a = a
    @b = b
    @c = c
  end

  def call
    # access with a/b/c using getter method instead of accessing @a/@b/@c directly
  end
  
  private
  
  # Define getter method in private if we dont want to access a, b, c outside of the class
  attr_reader :a, :b, :c

  def some_private_method
    # access with a/b/c using getter method instead of accessing @a/@b/@c directly
  end
end

So your class will be like this

class KeywordsQuery
  def initialize(user)
    @keywords = user.keywords
  end

  def call
    order_by_name
  end

  private

  attr_reader: keywords
  
  def order_by_name
    keywords.order(:name)
  end
end

[malparty]

Thanks for the example, though I'm not sure to understand the 'why'.

So you mean any single private instance variable should never be accessed directly, but always via accessors?
Is it to keep the flexibility of overriding the getter method later? Or any other reason?

I'm asking to be sure I understand the why -- back in C# I used to utilize getter/setters solely for public instance variables (called "Properties"). Here the why was specific to maintain a stable contract (API) so that consumers of the class won't have to change when the variable does (as getter will maintain a bridge). But for private variables, I'm not sure to see exactly what we want to tackle~

Though I have already refactored the code to use the accessor ;-)

Thanks @junan :)

[junan]

@malparty Theoretically we can access the instance variable directly inside the class, but we generally access them with accessors. I see the following benefit accessing value with accessor methods rather than directly accessing them with instance variable

• Defining attr_reader clearly tells us that those attributes are only for reading(not writing), so more clarity
• If we need to access those variables outside of class, we cant access them directly with the instance variable. We need to define the accessor. So It is better to use the accessor method always to access data, so more consistency
• Every time we access the variable value inside the class, we type one less character(@), so less typing

[malparty]

Thanks, very clear and I did not think of the attr-reader only situation!!! Cool! :)

[github-actions]

⚠️	Default_scope is evil

[github-actions]

⚠️	Default_scope is evil