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

[Defacepocalypse] De-deface product properties index #4074

Merged
merged 11 commits into from
Aug 28, 2019

Conversation

HugsDaniel
Copy link
Contributor

@HugsDaniel HugsDaniel commented Jul 24, 2019

What? Why?

There's no issue but it's part of #2807
Closes #1759

I imported and de-defaced views from spree/backend/app/views/spree/admin/product_properties. So now we have two more views :

  • app/views/spree/admin/product_properties/_product_property_fields.html.haml
  • app/views/spree/admin/product_properties/index.html.haml

Also added a %th.no-border to fix the weird shift between table headers and input fields.

Appart from that, no functional changes done, just bringing code from spree, converting to haml and applying the defaces.

What should we test?

Make sure this page works as before :

  • Product -> Edit -> Product Properties

Release notes

Changelog Category: Changed
Removed deface from a few parts of the backoffice thus making OFN less entangled with Spree code.

@HugsDaniel HugsDaniel self-assigned this Jul 24, 2019
@luisramos0
Copy link
Contributor

luisramos0 commented Jul 24, 2019

I found this last week, it works really well for erb-haml :-O
http://htmltohaml.com/

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

eheh, great to have you back Hugo!!!!

I think the js tag will not be working...

It would be good to also bring from spree and convert to haml the missing partials:

  • 'spree/admin/shared/edit_resource_links'
  • 'spree/admin/shared/product_sub_menu'
  • 'spree/shared/error_messages'

@HugsDaniel
Copy link
Contributor Author

It would be good to also bring from spree and convert to haml the missing partials:

  • 'spree/admin/shared/edit_resource_links'
  • 'spree/admin/shared/product_sub_menu'
  • 'spree/shared/error_messages'

Done!

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

💪 👍

app/views/spree/admin/product_properties/index.html.haml Outdated Show resolved Hide resolved
@RachL RachL self-assigned this Jul 31, 2019
@RachL
Copy link
Contributor

RachL commented Jul 31, 2019

@hugs I can add and edit properties. However, I cannot add them to products afterwards. I don't have a dropdown in /admin/products/test/product_properties
Here is what the dropdown currently looks like in french staging (without your PR):
image

And in staging ES with this PR I had this:

image

@HugsDaniel
Copy link
Contributor Author

Hey @RachL , how do you add properties to a product ?

About the dropdown, I have this in master currently :

Capture d’écran de 2019-07-31 17-05-22

No dropdown, just a regular autocomplete. Can you check master ?

@RachL
Copy link
Contributor

RachL commented Jul 31, 2019

😭 I hoped French staging had a recent version of master. So this means the bug was introduce somewhere else...

In production the dropdown is still here. So it is really a bug that we will introduce with the new release.
Any idea which one of the latest merged PR could have caused this? ping @luisramos0 @sauloperez @mkllnk

https://github.com/openfoodfoundation/openfoodnetwork/pulls?q=is%3Apr+merged%3A%3E2019-07-19T21%3A37%3A00%2B01%3A00+sort%3Aupdated-desc+base%3Amaster

About the properties, you can add them in /admin/properties @HugsDaniel

@RachL RachL added the blocked label Jul 31, 2019
@luisramos0
Copy link
Contributor

:-(
I am sorry, I am doing some pretty risky changes lately... let me say it's totally worth it, such a good clean up!
I'd bet on #4047 again...

@HugsDaniel
Copy link
Contributor Author

@RachL there's no dropdown but there is still autocomplete on the property's name. I'm not sure it's blocked however, since the bug has nothing to do with de-defacing. The HTML is pretty much the same as master, there's just a problem elsewhere with select2 dropdowns.

@RachL RachL added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 1, 2019
@RachL
Copy link
Contributor

RachL commented Aug 1, 2019

@HugsDaniel I've put the blocked label because once the dropdown is fixed, I don't know if the fix will have another impact on this PR. I just feel I cannot complete the test.

I will see if we squease a fix for the bug or not soon so I can re-test both.

@RachL RachL removed the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 1, 2019
@RachL
Copy link
Contributor

RachL commented Aug 2, 2019

@HugsDaniel I've just staged master as part of the release test and I do have the dropdown... Can you check master again on your side?
I noticed also that at least with the version we have here the column titles are better align. So actually if we just add a placeholder to explain the autocompletion, this is good to go?

@RachL RachL removed the blocked label Aug 2, 2019
@luisramos0
Copy link
Contributor

I found #1759 that is related to this PR. Maybe something to look at as part of this PR.

@HugsDaniel
Copy link
Contributor Author

@RachL stupid question but the dropdown appears only for non admin users, I guess you tested it with both types ?

@HugsDaniel
Copy link
Contributor Author

HugsDaniel commented Aug 22, 2019

I have this in this branch for a non admin user :
Capture d’écran de 2019-08-22 10-19-07

Seems to be working fine !

@HugsDaniel
Copy link
Contributor Author

@luisramos0 I tackled #1759 at the same time. @RachL can you test if the dropdown has two arrows on the right, like they have in my screenshot above ?

@RachL
Copy link
Contributor

RachL commented Aug 27, 2019

@HugsDaniel yes I do have the dropdown, but only one arrow:

image

So I guess you only need to tackle the unsucessful checks?

@sigmundpetersen
Copy link
Contributor

Rebuild passed on Semaphore, so those build errors are random. So I guess good to go then?

@luisramos0 luisramos0 merged commit 265e76e into openfoodfoundation:master Aug 28, 2019
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.

Confusing UX on Delete Properties
5 participants