-
Notifications
You must be signed in to change notification settings - Fork 5
Add ability to set read at on feedback #610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| response | ||
| rescue StandardError => e | ||
| Sentry.capture_exception(e) | ||
| response[:error] = response[:feedback]&.errors |
| response | ||
| rescue StandardError => e | ||
| Sentry.capture_exception(e) | ||
| response[:error] = response[:feedback]&.errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows existing patterns, so I think it's fine
| it 'returns the updated feedback' do | ||
| response = described_class.call(feedback: feedback) | ||
| expect(response[:feedback]).to eq(feedback) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused what this is testing to be honest. Might be simpler to test it is an instance of Feedback and then check the read at was set below like you're already doing
|
|
||
| context 'when set_read fails' do | ||
| before do | ||
| allow(feedback).to receive(:save!).and_raise(StandardError, 'Transition failed') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nitpicky sorry, but there might be a better example of an error message, just because Transition failed is quite specific to the state machine stuff
| it 'returns the original feedback' do | ||
| response = described_class.call(feedback: feedback) | ||
| expect(response[:feedback]).to eq(feedback) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same test as the one you have above, where response[:feedback] should have changed, so I'm confused
| it 'includes an error object in the response' do | ||
| response = described_class.call(feedback: feedback) | ||
| expect(response[:error]).to be_a(ActiveModel::Errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to test it returns the error you set above
| def set_read | ||
| feedback = Feedback.find(params[:id]) | ||
| result = Feedback::SetRead.call(feedback: feedback) | ||
|
|
||
| if result.success? | ||
| @feedback = result[:feedback] | ||
| render :show, formats: [:json], status: :ok | ||
| else | ||
| render json: { error: result[:error] }, status: :unprocessable_entity | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to 404 it if the feedback id is invalid?
Status
What's changed?