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

Create project and feed refactor #147

Closed
landonreed opened this issue Dec 7, 2018 · 14 comments
Closed

Create project and feed refactor #147

landonreed opened this issue Dec 7, 2018 · 14 comments

Comments

@landonreed
Copy link
Contributor

Issue by evansiroky
Wednesday Oct 11, 2017 at 05:24 GMT
Originally opened as catalogueglobal#41


Refactor the creation of projects and feed sources to occur on separate pages.

  • Created new routes for creation pages
  • Refactor create project and feed actions
  • New containers/components: CreateProject and CreateFeedSource
  • Refactor GeneralSettings component to be used by both Create Project and ProjectViewer
  • Tidy up and reuse code in various places

evansiroky included the following code: https://github.com/catalogueglobal/datatools-ui/pull/41/commits

@landonreed
Copy link
Contributor Author

Comment by codecov-io
Monday Oct 16, 2017 at 22:50 GMT


Codecov Report

Merging #41 into dev will increase coverage by 0.35%.
The diff coverage is 1.55%.

Impacted file tree graph

@@           Coverage Diff            @@
##             dev     #41      +/-   ##
========================================
+ Coverage   4.62%   4.98%   +0.35%     
========================================
  Files        325     328       +3     
  Lines      12092   15419    +3327     
  Branches    4333    4654     +321     
========================================
+ Hits         559     768     +209     
- Misses      9424   12488    +3064     
- Partials    2109    2163      +54
Impacted Files Coverage Δ
lib/manager/containers/ActiveProjectViewer.js 0% <ø> (ø) ⬆️
lib/editor/components/FareRulesForm.js 0% <ø> (ø) ⬆️
lib/editor/components/EditorHelpModal.js 0% <ø> (ø) ⬆️
lib/editor/components/VirtualizedEntitySelect.js 0% <ø> (ø) ⬆️
lib/main.js 0% <ø> (ø) ⬆️
lib/editor/components/MinuteSecondInput.js 0% <ø> (ø) ⬆️
lib/types/index.js 0% <ø> (ø) ⬆️
lib/public/components/FeedsMap.js 0% <ø> (ø) ⬆️
lib/admin/components/UserSettings.js 0% <ø> (ø) ⬆️
lib/editor/components/timetable/HeaderCell.js 0% <ø> (ø) ⬆️
... and 343 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66fb3bc...229262e. Read the comment docs.

@landonreed
Copy link
Contributor Author

Comment by landonreed
Tuesday Oct 17, 2017 at 13:55 GMT


@evansiroky here's a mockup of where CreateFeedSource might work better (i.e., replacing the FeedSourceTable in ProjectViewer).

image

@landonreed
Copy link
Contributor Author

Comment by evansiroky
Tuesday Oct 17, 2017 at 18:53 GMT


I'll refactor the CreateFeedSource containers and components so that it'll replace the FeedSourceTable.

@landonreed
Copy link
Contributor Author

Comment by evansiroky
Monday Jul 30, 2018 at 21:12 GMT


@landonreed did we still want to include this?

@landonreed
Copy link
Contributor Author

Comment by landonreed
Tuesday Jul 31, 2018 at 16:18 GMT


@evansiroky, I think we should include it still, but I must admit I'm a bit afraid of what the merge conflicts will reveal.

@landonreed
Copy link
Contributor Author

Comment by evansiroky
Saturday Oct 20, 2018 at 03:59 GMT


No more merge conflicts. Should be ready to go.

@landonreed
Copy link
Contributor Author

Comment by landonreed
Tuesday Nov 06, 2018 at 22:54 GMT


I think I just discovered another larger issue, @evansiroky. It appears that the when updating a project, ProjectSettingsForm writes the entire project object (with nested feed sources and any other object that has been added within the store) to the controller. This is not desired because we'll end up with a list of feed sources in mongo for a given project document that should only actually exist in the feed sources collection.

Can you confirm that this is the case?

@landonreed
Copy link
Contributor Author

Comment by evansiroky
Thursday Nov 08, 2018 at 21:05 GMT


I think I just discovered another larger issue, @evansiroky. It appears that the when updating a project, ProjectSettingsForm writes the entire project object (with nested feed sources and any other object that has been added within the store) to the controller. This is not desired because we'll end up with a list of feed sources in mongo for a given project document that should only actually exist in the feed sources collection.

Can you confirm that this is the case?

Confirmed and fixed. PR should be ready to be merged.

@landonreed
Copy link
Contributor Author

Comment by landonreed
Monday Nov 12, 2018 at 19:17 GMT


@evansiroky, some further changes are needed:

  • defaultLanguage field should be removed from the UI form (and flow type def)
  • when I update settings for a project that has feed sources and then go back to the Feed Sources tab, the feed sources are missing

@landonreed
Copy link
Contributor Author

Comment by evansiroky
Tuesday Nov 13, 2018 at 06:35 GMT


@evansiroky, some further changes are needed:

  • defaultLanguage field should be removed from the UI form (and flow type def)
  • when I update settings for a project that has feed sources and then go back to the Feed Sources tab, the feed sources are missing

These items should be addressed in the latest changes. Lmk if anything else needs fixing.

@landonreed
Copy link
Contributor Author

Comment by landonreed
Thursday Nov 15, 2018 at 18:51 GMT


Thanks @evansiroky, that looks good. I had a few other changes in mind that I've put in #255. Feel free to review that and merge into this branch if they look good to you. After that, I think this is good to merge.

@landonreed
Copy link
Contributor Author

Comment by evansiroky
Tuesday Nov 20, 2018 at 06:10 GMT


Reassigning to Landon as #255 has some requested changes. Also, need the requesting changes review status to change to approved. :)

@landonreed
Copy link
Contributor Author

Comment by landonreed
Monday Dec 03, 2018 at 21:49 GMT


@evansiroky, could you fix up those conflicts so that I can merge?

@landonreed
Copy link
Contributor Author

🎉 This issue has been resolved in version 4.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

No branches or pull requests

2 participants