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

Renamed all references of paid to free #379

Merged
merged 8 commits into from
Oct 19, 2020
Merged

Conversation

Kandeel4411
Copy link
Collaborator

@Kandeel4411 Kandeel4411 commented Sep 30, 2020

Change 'paid' to 'free

Renamed all references of paid to free and created a migration to reflect the renaming. This should hopefully account for everything!

About updating the old data, I think that should be relatively easily done on your side through flask shell by querying current resources and negating their current boolean values, what do you think of this?

@@ -68,7 +68,7 @@ def format_resource_search(hit):
'url': hit['url'],
'category': hit['category'],
'languages': hit['languages'],
'paid': hit['paid'],
'free': hit['free'],
Copy link
Member

Choose a reason for hiding this comment

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

oh snap, I forgot, this change is going to require us to do something about Algolia... I wonder if re-indexing will be enough? Anyway, nothing for you to do here, just wanted to make a note for myself that once we merge this and deploy we'll have to fix that

Copy link
Member

Choose a reason for hiding this comment

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

I totally called it, seeing this in production

[2020-10-21 15:34:32,534] ERROR in app: Exception on /api/v1/search/ [GET]
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/flask/app.py", line 2447, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.7/site-packages/flask/app.py", line 1952, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.7/site-packages/flask/app.py", line 1821, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/usr/local/lib/python3.7/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python3.7/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "./app/api/routes/search.py", line 14, in search
    return search_results()
  File "./app/api/routes/search.py", line 72, in search_results
    results = [utils.format_resource_search(result) for result in search_result['hits']]
  File "./app/api/routes/search.py", line 72, in <listcomp>
    results = [utils.format_resource_search(result) for result in search_result['hits']]
  File "./app/utils.py", line 70, in format_resource_search
    'free': hit['free'],
KeyError: 'free'

I'm not sure exactly what to do to fix it... I assume reindex everything in algolia, but I'm not sure which commands to run to get there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure as well about this as I have no experience in it but I think this could be the solution/related? https://www.algolia.com/doc/api-reference/api-methods/move-index/?language=python

aaron-junot
aaron-junot previously approved these changes Oct 2, 2020
Copy link
Member

@aaron-junot aaron-junot left a comment

Choose a reason for hiding this comment

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

I like these changes. I want to coordinate with the front-end folks to get this change made over there so that it doesn't break the page for any length of time. So I'm not going to merge until I do that, but otherwise, it looks good. Thanks for this!

@Kandeel4411
Copy link
Collaborator Author

Yeah, it would be best to check with the frontend first. Always happy to help :)!

@Kandeel4411 Kandeel4411 changed the title Renamed all references of paid to true Renamed all references of paid to free Oct 2, 2020
@aaron-junot aaron-junot requested a review from kylemh October 9, 2020 17:45
@aaron-junot
Copy link
Member

@kylemh What work needs to be done on the front end to change paid to free? Should we make a separate issue on the front end repo and link this PR to it? Then merge both and deploy both all at once?

We should really decide on a proper process for getting in changes that affect the front end and the API. This way we don't have to re-invent the wheel when this type of thing comes up again. We'll already know what to do.

@kylemh
Copy link
Member

kylemh commented Oct 9, 2020

Looks like the only changes you would need to make are in pages/resources/[page].js. There are a few references to paid via a query string look-up + assuming it comes along for the ride of the response for each object.

A change you could do on the front-end now would be to deal with paid OR free so it's backwards compatible for back-end, but we still havent announced or shared this page, so I'm okay with breaking it as long as we fast follow with the FE changes such that integration tests pass

@aaron-junot
Copy link
Member

I kind of hope that the integration tests would be checking for this as a breaking change. I'll make an issue for the front end to do paid or free so that way we can just deploy this any time after that change is made. Then we can follow up with a final front end ticket to remove the paid option

@kylemh
Copy link
Member

kylemh commented Oct 9, 2020

yeah, if u deploy this - cypress should have many failed tests

@kylemh
Copy link
Member

kylemh commented Oct 18, 2020

Can we get this deployed to both staging and prod?

@aaron-junot aaron-junot merged commit 34e7f70 into main Oct 19, 2020
@aaron-junot aaron-junot deleted the rename-paid-column branch October 19, 2020 22:58
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.

3 participants