Skip to content

Add update method to document controller - #329

Open
Creiwry wants to merge 16 commits into
Mate2xo:developmentfrom
Creiwry:add-update-method-to-document-controller
Open

Add update method to document controller#329
Creiwry wants to merge 16 commits into
Mate2xo:developmentfrom
Creiwry:add-update-method-to-document-controller

Conversation

@Creiwry

@Creiwry Creiwry commented Jan 16, 2023

Copy link
Copy Markdown
Collaborator

Tickets: #315 and #316

Added features:

  • edit document view
  • update method in documents controller
    • update name of Activestorage::blob

    • update category

  • locales for documents view
  • rspec test for update method

@Mate2xo Mate2xo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't finished yet, I'll make antother review later. Good work so far, thanks

Comment thread app/controllers/documents_controller.rb Outdated
Comment thread app/transactions/documents/update_transaction.rb Outdated
Comment thread config/locales/views/documents.fr.yml Outdated
Comment thread app/views/documents/edit.html.erb Outdated
Comment thread app/views/documents/edit.html.erb Outdated
Comment thread app/views/documents/_index.html.erb Outdated
Comment thread app/views/documents/_index.html.erb Outdated
Comment thread app/views/documents/edit.html.erb Outdated
Comment thread config/locales/views/documents.en.yml Outdated
Comment on lines +3 to +4
document_name: File name
download_file: Download

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref the the previous .key.lookup comment before ;
download_file sould probably be under the index key

@Creiwry
Creiwry requested a review from Mate2xo January 30, 2023 22:15
params.require(:document).permit(:category, :file, :file_name)
end

def update_transaction # removed the equals thing here, don't know if it still works

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment does not gives relevant information to the reader without context. It's better to delete it.
Btw, if you don't know if it still works, then you should write a test to check that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, my bad. Made the comment for myself but forgot to delete

Comment thread app/controllers/documents_controller.rb Outdated
Comment thread app/transactions/documents/update_transaction.rb Outdated
Comment thread config/locales/views/documents.en.yml Outdated
Comment thread config/locales/views/documents.fr.yml Outdated
@Creiwry
Creiwry requested a review from Mate2xo February 3, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants