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.

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

Add "Additional Information" Field to Items for Bank Use Only #4643

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TheFaizanAnwar
Copy link

Resolves #4527

Description

This PR introduces an additional additional_info text field to the Items module, specifically for bank users. This field is added to the "new", "edit", "view", and "index" pages, and is only visible to users with bank-specific access. The purpose of this field is to allow banks to store/access particular information about items that partners do not need to see.

In the index view, the column header is shortened to "Add. Info" to prevent layout issues. Additionally, the text in this column is truncated to 25 characters to maintain the integrity of the table layout. Tests have been added to ensure this feature functions correctly.

Type of change

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

How Has This Been Tested?

  • Tests have been added to spec/controllers/items_controller_spec.rb to verify CRUD operations for the additional_info field.
  • The truncation functionality in the index view was manually verified to ensure it doesn't affect the table layout.

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 ok on manual testing. @dorner - can you take a look from a technical pov?

@cielf cielf requested a review from dorner September 9, 2024 14:38
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

One question - otherwise looks good.

@@ -52,6 +52,12 @@
<%= f.input :visible, label: "Item is Visible to Partners?", wrapper: :input_group do %>
<%= f.check_box :visible_to_partners, {class: "input-group-text", id: "visible_to_partners"}, "true", "false" %>
<% end %>

<% if current_user.has_role?(Role::ORG_ADMIN, current_organization) || current_user.has_role?(Role::ORG_USER, current_organization) %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this check is not necessary because only org users can get to this page. Same with the other partials. @cielf can you confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only bank users can see this page, so we don't need a finer-grained control at this time.

Copy link
Author

Choose a reason for hiding this comment

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

Noted. I will remove those check.

@dorner
Copy link
Collaborator

dorner commented Sep 12, 2024

Also a bunch of failing tests :(

@TheFaizanAnwar
Copy link
Author

Also a bunch of failing tests :(

I've tested it locally using bundle exec rspec and confirmed that all tests are passing. I will fix those failing test.

Thanks, @dorner and @cielf, for your review. This is my first PR for this repo.

@cielf
Copy link
Collaborator

cielf commented Sep 28, 2024

@TheFaizanAnwar FYI: We had an urgent fix that required all the senior contributors this week , so we didn't get to look at this again. Hopefully this week will go better.

@TheFaizanAnwar
Copy link
Author

Sure I will fix it asap.

@TheFaizanAnwar
Copy link
Author

@cielf, I hope you're doing well. I have made the necessary changes based on your feedback on PR.

Additionally, all the test cases are passing on my local environment, and I’ve attached a screenshot for your reference. I hope the tests will pass in the remote environment as well.
image

@cielf
Copy link
Collaborator

cielf commented Nov 13, 2024

Hey @dorner -- I think this one is in your court.

@dorner
Copy link
Collaborator

dorner commented Nov 16, 2024

@TheFaizanAnwar the tests are still failing. There are also conflicts now that have to be resolved.

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.

[Feature request] additional information field on items
3 participants