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

Fix bug #12835 for producer reports #12847

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Fix bug #12835 for producer reports #12847

merged 3 commits into from
Sep 19, 2024

Conversation

dacook
Copy link
Member

@dacook dacook commented Sep 5, 2024 •
edited
Loading

What? Why?

Although it was hard to get the tests right, in the end it turned out to be a simple fix!

What should we test?

As per issue:

  1. Set up a Producer A with profile only and a product (the user should not own another shop).
  2. Setup a hub B and create an order cycle with product of Producer A
  3. Create at least an order
  4. Check that Hub B can display reports, but A can not

@dacook dacook force-pushed the fix-bug-12835 branch 2 times, most recently from bdaecfb to f7a1f97 Compare September 17, 2024 02:17
@dacook dacook changed the title update specs Fix bug #12835 for producer reports Sep 17, 2024
@dacook dacook added bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. user facing changes Thes pull requests affect the user experience labels Sep 17, 2024
@dacook
Update specs
1078e7c 
The key here is the enterprise_relationship. This is required for the supplier to have permission to see the orders.

Curiously, the unit test still passes. All will be revealed in the next commit..
@dacook dacook marked this pull request as ready for review September 17, 2024 02:56
@rioug rioug self-requested a review September 17, 2024 03:11
rioug
rioug requested changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good, but the Ability spec should also be updated.

app/models/spree/ability.rb Show resolved Hide resolved
mkllnk
mkllnk approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great that we have a spec for it now. And thanks for fixing my mistakes.

app/models/spree/ability.rb Show resolved Hide resolved
@mkllnk mkllnk requested a review from rioug September 17, 2024 04:10
@filipefurtad0 filipefurtad0 self-assigned this Sep 19, 2024
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 19, 2024
@filipefurtad0
Copy link
Contributor

Hey @dacook ,

I've ran a quick test on this one:

image

The issue is fixed, reports are accessible to profile only users.

Merging.

@filipefurtad0 filipefurtad0 merged commit 2809194 into openfoodfoundation:master Sep 19, 2024
53 checks passed
@RachL RachL removed the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@rioug rioug rioug approved these changes

Assignees

@dacook dacook

@filipefurtad0 filipefurtad0

Labels
bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. user facing changes Thes pull requests affect the user experience
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Asking a report has no result when the producer is set as profile only (doesn't sell products)
5 participants
@dacook @filipefurtad0 @mkllnk @rioug @RachL