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

Deprecate legacy views #5307

Merged
merged 1 commit into from
Apr 18, 2019
Merged

Conversation

alonpeer
Copy link
Contributor

@alonpeer alonpeer commented Mar 29, 2019

Fixes #3531 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@alonpeer
Copy link
Contributor Author

alonpeer commented Mar 29, 2019

@publiclab/reviewers @jywarren please review.

Note:
This PR builds on top of #5338, so this can only get merged after the other one does.
Please only review the newest commit. The other two are from the other PR.

@plotsbot
Copy link
Collaborator

plotsbot commented Mar 29, 2019

1 Warning
⚠️ New migrations added. Please update schema.rb.example by overwriting it with a copy of the up-to-date db/schema.rb. Also, be aware to preserve the MySQL-specific conditions for full-text indices.
1 Message
📖 @alonpeer Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@SidharthBansal
Copy link
Member

There are some conflicts. Please remove them so that we can review it.
Thanks for contributing at PL

@alonpeer
Copy link
Contributor Author

@SidharthBansal it'll be easier for me to resolve the conflicts after #5338 is merged, since this PR depends on it. The conflicts are around the db schema, and since multiple PRs can change that file with migrations, I'd like to avoid chasing these conflicts and just do it once.
Once #5338 is merged, I'll follow up with resolving conflicts here.

@SidharthBansal
Copy link
Member

SidharthBansal commented Apr 12, 2019 via email

Copy link
Member

@SidharthBansal SidharthBansal left a comment

Choose a reason for hiding this comment

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

Hi, changes looks good to me. Thanks for your work.@gauravano can you please review this and merge this?

@SidharthBansal
Copy link
Member

The conflicts should be resolved once @gauravano you will review and merge the #5338. These prs have conflicting code snippets.
Thanks for the work @alonpeer. I really appreciate your help.

@jywarren
Copy link
Member

It looked like a timestamp needed to be updated! I made the change, let's see how it does!

@alonpeer
Copy link
Contributor Author

@jywarren 👍 thanks!
I would imagine that once #5338 is merged, it'll be nice to rebase this branch again on master, so we get a nice, clean history. I can do that once the other PR is merged.

@alonpeer alonpeer force-pushed the deprecate-legacy-views branch from acf3f00 to 32c788e Compare April 18, 2019 21:29
@alonpeer alonpeer changed the title [DO NOT MERGE] Deprecate legacy views Deprecate legacy views Apr 18, 2019
@alonpeer
Copy link
Contributor Author

@jywarren I've rebased and updated the migration file name. This should now be ready 👍

@jywarren jywarren merged commit f34e679 into publiclab:master Apr 18, 2019
@jywarren
Copy link
Member

Great!!! And.... done! Fantastic work, thank you @alonpeer !!!

@grvsachdeva
Copy link
Member

Hey @alonpeer, would you be interested in being a mentor for GSoC program - #4585?

Thanks!

@alonpeer
Copy link
Contributor Author

Hey @gauravano ! Wow, thanks for this honour, I really appreciate it 😄
I will, however, have to decline. Due to personal reasons, I’m going to have to take a break from contributing, since I will be quite busy in the next few months.
I will try to pop by and review a PR here and there, but can't promise more than that.

Cheers!

@grvsachdeva
Copy link
Member

Hey @alonpeer, it's fine.

Also, like to mention that we really appreciate the awesome work you do with us and help us grow. I personally found your PRs very interesting and definitely learn from them :)

Best,
Gaurav

@alonpeer
Copy link
Contributor Author

@gauravano you are VERY kind! I'm really happy to be part of this wonderful community 🎉

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.

Results are not sorted according to views
5 participants