848 - Merge OrganizationProfile with Organization

[sarvaiyanidhi]

🔗 Issue

848

✍️ Description

• Migrated OrganizationProfile fields into Organization table
• Updated Model, Controller, Policy, Routes with relevant changes
• Updated test cases
• Removed OrganizationProfile reference across the application

📷 Screenshots/Demos

NA

[kasugaijin]

@sarvaiyanidhi thank you! I am away this weekend so unless someone beats me to it, I won't review this until next week.

[sarvaiyanidhi]

@sarvaiyanidhi thank you! I am away this weekend so unless someone beats me to it, I won't review this until next week.

Sure @kasugaijin No worries. Will await for review from you or someone else. Thanks

[sarvaiyanidhi]

Thank you @mononoken for the feedback. I have gone through all comments and will make requested changes. Will push the revised code soon.

[mononoken]

Hi, @sarvaiyanidhi ! I saw you made some updates, which skimming over, look good. However, some tests are not passing. Are you still working on this? Just comment here whenever it's ready for review or click to re-request review. Let us know if you need help with any parts 😊

[sarvaiyanidhi]

Hi, @sarvaiyanidhi ! I saw you made some updates, which skimming over, look good. However, some tests are not passing. Are you still working on this? Just comment here whenever it's ready for review or click to re-request review. Let us know if you need help with any parts 😊

Hi @mononoken, I have implemented all changes related to location association changes and also made necessary changes as suggested for Avatar deletion to work with organization.

I am facing issue with test for attachment_policy.rb and attachment_controller.rb files after making those changes and trying to fix it. I did try to make related changes in test files to change from profile to organization but still not able to fix it. If you can provide pointers on 4 failing test cases related to it it will help me to proceed further.

[mononoken]

@sarvaiyanidhi The problem from the tests seems to stem from how AttachmentPolicy#organization is working. The policy is trying to verify the organization associated with the attachment matches the user's org.

I had suggested you update the #organization method because of how Organization has been refactored. Since OrganizationProfile is gone, Organization itself now has attachments. However, that suggestion was incomplete because orgs are not the only things with attachments. Pets also have attachments for example. So the fix I suggested fixed Org attachments but broke all the other ones.

  # app/policies/active_storage/attachment_policy.rb
  def organization
    @organization || record_organization
  end

  def record_organization
    if record.record.is_a?(Organization)
      record.record
    else
      record.record.organization
    end
  end

This code adapts the policy to both cases. If the attachment's record is itself an Org, it uses the record. If it is not, it checks that the attachment's record has an associated org and uses that.

If you could take a few minutes to double check that I am correct and see if you can come up with a better approach that would be great! I don't like using is_a? but I think it's probably fine and the simplest solution I can think of in this case.

[sarvaiyanidhi]

Thank you @mononoken .. It does solves the problem.

How about this alternate code suggestion?

  def organization
    @organization || record_organization
  end

  def record_organization
    (record.record.respond_to?(:organization) ? record.record.organization : record.record)
  end

Do you recommend this or the one you suggested?

[mononoken]

~I prefer my suggested approach. The only time we want organization === record.record is if it is an Organization, so having record.record in the else of that ternary feels a little wrong to me. I think the is_a? makes the intentions clearer too. However, both will work for this case, so I will leave it to you to decide which you prefer to push 🙂~

~You could also change my if to a ternary if you choose that path. However, if you use a ternary, I would lose the outer parentheses. They're not necessary for this statement.~

We discussed this one in the meeting a bit, and we decided to go with the approach that I included. We decided it was more readable. Thank you for providing your suggestion too!

Ben thought it would be good to add a comment as well for the method. Something like:

# `record` should be an ActiveStorage Attachment which responds to `#record`.
def record_organization

[sarvaiyanidhi]

Just to let you know, one bug has surfaced from these changes. The locations are associated with both AdopterFosterProfiles and Orgs, but this PR only associates the locations with the Organizations. It also forces this association using the acts_as_tenant method.

I will be writing a new issue to address this concern. If you want to work on that one next, I would be happy to assign it to you. No pressure though if you want to work on something else. I'll try to get that one typed up today.

Hi @mononoken ,

Sure, I can work on this related issue and get it done. Can you please assign new issue to me once you add it. Thanks!

[sarvaiyanidhi]

Hey @mononoken ,

As per earlier comment, I think we don't need new issue now for AdopterFosterProfiles and location association issue as I saw recently issue on removing AdopterFosterProfiles. Let me know if you are still looking for any other changes related to it, I can work on it.

Thanks

[mononoken]

Hey @mononoken ,

As per earlier comment, I think we don't need new issue now for AdopterFosterProfiles and location association issue as I saw recently issue on removing AdopterFosterProfiles. Let me know if you are still looking for any other changes related to it, I can work on it.

Thanks

Hi, @sarvaiyanidhi ! So sorry I did not get back to you on this. I've been MIA for a few weeks. You are right in your observation that we are removing AdopterFosterProfile now, so we won't need to make that change after all. Sorry again for any confusion this may have caused you!