ChaelCodes / MeetAnotherDay

An app to help you find and meet up with your friends at conferences.
MIT License
35 stars 20 forks source link

Remove name and bio from user table #184

Closed fransan6 closed 3 months ago

fransan6 commented 5 months ago

Description of Feature or Issue

closes #182

As per the Issue, neither name nor bio are needed in the user table. I created a migration to remove these fields from the table.


Note In addition, I removed references to a user's bio or name. I wasn't sure if whole sections should be deleted; for example, in views/users/show.html.erb, I removed just user.bio and user.name but left the <p> and <strong> tags. In addition, I left views/users/_form.html.erb as is and did not delete the name and bio form fields.

I also edited RSpec as removing these table fields created over 240 failures.

Neither removing references to a user's bio or name, or updating RSpec was included in the Issue so please let me know if you would like to me to undo these changes.


User-Facing Changes

ChaelCodes commented 5 months ago

Just wanted to give you some quick feedback and thoughts before a more thorough review later.

Great call on updating the factory/specs and removing those fields in the views! I'd love to see the strong and

tags removed and the form updated too.

Your PR has me thinking more about the purpose of those views and if they're even necessary. I can envision a world where we only have a show page (that's only visible to the user) that links to devise views for changing the password and/or email. Ideally creation and updating is handled by devise, and an index page of all users feels odd. At minimum it would be an admin-only view.

All this feels like a second issue and definitely outside the scope of this one though!

fransan6 commented 5 months ago

OK, thank you for the quick feedback and yes, as I was going through this and potentially deleting most/all content on user pages, it did seem like this might be a bigger issue..!

ChaelCodes commented 3 months ago

Hey! It's been a while. I should've checked in with you before now. Are you able to see the linter error in Rubocop? I was hoping to have that passing before merge.

fransan6 commented 3 months ago

Hey! No worries or rush. When I check the details of the lint, it confirms:

Offenses:

db/migrate/202402021[8](https://github.com/ChaelCodes/Untitled-Friend-App/actions/runs/7768895600/job/21187348566?pr=184#step:5:9)3823_remove_name_and_bio_from_users.rb:6:5: C: Rails/BulkChangeTable: You can use change_table :users, bulk: true to combine alter queries.
    remove_column :users, :name, :string
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To remove the error, these edits display no Rubocop file errors:

class RemoveNameAndBioFromUsers < ActiveRecord::Migration[7.1]
  def change
    change_table :users, bulk: true do |t|
      t.remove :name, type: :string
      t.remove :bio, type: :string
    end
  end
end

I added type: string from here as Rubocop stated: t.remove (without type) is not reversible. Let me know if you'd like me to commit this!

For easy reference, this is the current migration:

class RemoveNameAndBioFromUsers < ActiveRecord::Migration[7.1]
  def change
    remove_column :users, :name, :string
    remove_column :users, :bio, :string
  end
end
ChaelCodes commented 3 months ago

@fransan6 That looks great! Please commit that and we'll get this shipped.

fransan6 commented 3 months ago

Sure, done!

fransan6 commented 3 months ago

Thanks for your help and patience 😊