Skip to content
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.

Sign up for GitHub

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

Jump to bottom

4407-showing all the item fields in the item view #4431

Merged
merged 2 commits into from
Jun 26, 2024
Merged

4407-showing all the item fields in the item view #4431

merged 2 commits into from
Jun 26, 2024

Conversation

Naraveni
Copy link
Contributor

@Naraveni Naraveni commented Jun 8, 2024

Resolves #4407

Description

All Item fields are shown now in Item view.

Guide questions:

  • What motivated this change (if not already described in an issue)?
  • So the banks have one stop for all the info on the item.

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

Screenshot 2024-06-08 at 9 05 04 AM

cielf
cielf requested changes Jun 9, 2024
View reviewed changes
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Thank you for this! I did some light manual testing, and there are a couple of minor issues to be addressed before we can put it forward.

app/views/items/show.html.erb Show resolved Hide resolved
app/views/items/show.html.erb Outdated Show resolved Hide resolved
@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch from 42d1351 to 2f994b8 Compare June 9, 2024 12:09
@Naraveni Naraveni marked this pull request as draft June 9, 2024 12:10
@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch from 2f994b8 to c1d9716 Compare June 9, 2024 12:11
@Naraveni Naraveni requested a review from cielf June 9, 2024 12:11
@Naraveni Naraveni marked this pull request as ready for review June 9, 2024 12:12
cielf
cielf requested changes Jun 11, 2024
View reviewed changes
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Looks like we had a miscommunication. It'll be easy to fix, though.

app/views/items/show.html.erb Outdated Show resolved Hide resolved
@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch from 16ca584 to 40c61ce Compare June 13, 2024 13:39
@Naraveni Naraveni requested a review from cielf June 13, 2024 13:41
@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch 3 times, most recently from 3b00ae5 to 5000dd7 Compare June 13, 2024 18:12
cielf
cielf reviewed Jun 13, 2024
View reviewed changes
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

LGTM from a functional pov. Can you add some automated tests demonstrating that the fields get shown? (it doesn't look like we have any tests for this view -- but I believe they would go in spec/requests/items_requests_spec.rb)

@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch from 5000dd7 to 78886b7 Compare June 17, 2024 21:16
@Naraveni Naraveni requested a review from cielf June 17, 2024 21:17
cielf
cielf previously requested changes Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @Naraveni -- thanks for the tests! I've got a few suggestions. Once those are addressed, I'll raise it to our top tech guy for a look.

spec/requests/items_requests_spec.rb Outdated Show resolved Hide resolved
spec/requests/items_requests_spec.rb Show resolved Hide resolved
spec/requests/items_requests_spec.rb Outdated Show resolved Hide resolved
spec/requests/items_requests_spec.rb Outdated Show resolved Hide resolved
@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch from 1770dfc to bdbb497 Compare June 21, 2024 04:28
@Naraveni Naraveni requested a review from cielf June 21, 2024 04:28
@cielf cielf dismissed their stale review June 23, 2024 00:54

My concerns have been addressed -- sending it over to @dorner for a more stringent technical review.

@cielf cielf requested a review from dorner June 23, 2024 00:54
dorner
dorner requested changes Jun 23, 2024
View reviewed changes
app/views/items/show.html.erb Outdated Show resolved Hide resolved
app/views/items/show.html.erb Outdated Show resolved Hide resolved
app/views/items/show.html.erb Outdated Show resolved Hide resolved
spec/requests/items_requests_spec.rb Outdated Show resolved Hide resolved
@Naraveni Naraveni force-pushed the 4407-INCLUDING-ALL-FIELDS-IN-ITEM-VIEW branch from de25cdc to 065abc8 Compare June 26, 2024 01:07
@Naraveni Naraveni requested a review from dorner June 26, 2024 01:12
@dorner
Copy link
Collaborator

dorner commented Jun 26, 2024

@Naraveni thanks! (For next time, please don't force push branches that have active reviews, as it makes reviewing the new changes harder.)

@dorner dorner merged commit 39ad8ed into rubyforgood:main Jun 26, 2024
19 checks passed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 7, 2024

@Naraveni: Your PR 4407-showing all the item fields in the item view is part of today's Human Essentials production release: 2024.07.07.
Thank you very much for your contribution!

@awwaiid awwaiid added this to the Request Units (Packs) milestone Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dorner dorner dorner approved these changes

@cielf cielf Awaiting requested review from cielf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Request Units (Packs)
Development

Successfully merging this pull request may close these issues.

[PACKS] #12 Item view should include all fields (including new custom request units)
4 participants
@Naraveni @dorner @cielf @awwaiid