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

Sort products alphabetically in OC edit page #4454

Merged
merged 3 commits into from
Nov 27, 2019

Conversation

luisramos0
Copy link
Contributor

What? Why?

This makes the list of products in the OC page sorted alphabetically.

What should we test?

Verify the order of the products is alphabetical.

Release notes

Changelog Category: Changed
The lists of products in the OC create/edit pages are sorted alphabetically.

@sauloperez
Copy link
Contributor

In this case, I wonder if it's worth adding or tweaking an existing test to ensure the ordering 🤔

@luisramos0
Copy link
Contributor Author

rebased to resolve conflicts.

@mkllnk
Copy link
Member

mkllnk commented Nov 12, 2019

I restarted the build. If it passes, it can go to test ready.

@luisramos0
Copy link
Contributor Author

looks like #4422 is looking good so this can now be tested.

This PR should not be merged before #4422, we need to merge #4422 and then switch the target branch of this PR. But we can test this meanwhile.

@RachL RachL assigned RachL and unassigned JenRSmith Nov 20, 2019
@RachL RachL added the pr-staged-au staging.openfoodnetwork.org.au label Nov 20, 2019
@RachL
Copy link
Contributor

RachL commented Nov 20, 2019

@JenRSmith I'm sorry I will test this one as well because it's kind of a package with the new OC page.

@RachL
Copy link
Contributor

RachL commented Nov 20, 2019

@luisramos0 do you know why the product "LA TENDA" is on top (AU staging - DorV Online OC)? :

image

@RachL
Copy link
Contributor

RachL commented Nov 20, 2019

@luisramos0 funfact : this product i son top but only in outgoing, not in incoming :) This is a screenshot of incoming:

image

@luisramos0 luisramos0 changed the base branch from the_poc to master November 20, 2019 21:16
The products are now coming from the server already sorted
@luisramos0
Copy link
Contributor Author

hey @RachL
there's a trailing space in that product name:
Screenshot 2019-11-26 at 17 13 05

Removing the space will move the product to its place in the list.
In the incoming exchanges page, that space gets trimmed in some way. I dont understand why it's not at the top of the list on the incoming exchanges...
When I try to replicate this with a product with a trailing space in the name, I get the product at the top of the list in both incoming and outgoing...
I think we can ignore this. Removing the trailing space will move the product to its place in the list.

Additionally but I think unrelated:
Investigating this I just realized before this PR the products were already sorted alphabetically on the outgoing exchanges (done on the browser).
This PR is sorting all products on the server so I added a new commit to remove the existing, and now unnecessary, sorting done on the browser.

@RachL
Copy link
Contributor

RachL commented Nov 27, 2019

Alright I've restaged this and indeed, couldn't reproduce! This is ready to go.

@RachL RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Nov 27, 2019
@luisramos0 luisramos0 merged commit e0e833b into openfoodfoundation:master Nov 27, 2019
@luisramos0 luisramos0 deleted the sort_products branch November 27, 2019 22:27
@RachL
Copy link
Contributor

RachL commented Feb 24, 2020

This isn't working anymore 😢
@luisramos0 do you know if the latest changes have something to do with it? I will prepare an issue

@luisramos0
Copy link
Contributor Author

I am sorry to hear that.
I dont see how the new changes could break this. The ordering is still there.
Please create an issue, I'll have a look.

@RachL
Copy link
Contributor

RachL commented Feb 25, 2020

@luisramos0 I did one here: #4845 is it something we can tag good first issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants