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

[LOI Editor] Imported LOIs not shown on map when importing for new unpublished job #1714

Closed
gino-m opened this issue Apr 9, 2024 · 13 comments
Assignees
Labels
type: bug Something isn't working

Comments

@gino-m
Copy link
Collaborator

gino-m commented Apr 9, 2024

@nwkotto @DaoyuT @rfontanarosa

@gino-m gino-m added the type: bug Something isn't working label Apr 9, 2024
@gino-m gino-m added this to the Beta release (18-Jul-24) milestone Apr 17, 2024
@DaoyuT DaoyuT moved this to Todo in Ground Apr 25, 2024
@DaoyuT DaoyuT moved this from Todo to In Progress in Ground Apr 25, 2024
@jcqli
Copy link

jcqli commented May 20, 2024

@DaoyuT What is the proposed plan for this change?

@DaoyuT
Copy link
Contributor

DaoyuT commented Jul 1, 2024

To reproduce

  • Create a new job
  • Import LOIs for that new job

Expect to see the imported LOIs on the map, but they don't show up.

Reason

When the job is not published, those LOIs are uploaded but associated with any job, so the map component won't render them.

Proposed solution

2 ways depends on if we want to bundle the import/delete LOI action within the publish job action:

If we want them together(more complex fix)

  • Add a state to each LOI and manage the states so that in the edit job phase
    • user import LOIs, LOIs shows in edit page, user clicks publish button, LOIs shows in map page
    • user import LOIs, LOIs shows in edit page, user quits editing without clicking publish, LOIs does not show in map page
    • user delete LOIs, LOIs does not show in edit page, user clicks publish button, LOIs does not show in map page
    • user delete LOIs, LOIs does not show in edit page, user quits editing without clicking publish, LOIs shows in map page
    • same behavior should happen to a new unpublished job

If we are okay with them separated(easier fix)

  • Make sure the imported LOIs for the new unpublished job are associated with the job id and can reach the map component

@jcqli
Copy link

jcqli commented Jul 1, 2024

FYI @gino-m , more context above. WDYT about the easier solution?

@gino-m
Copy link
Collaborator Author

gino-m commented Jul 2, 2024

Hi @DaoyuT @jcqli It sounds like we won't have time for the full fix by Beta launch this month; let's go for option 2, and file a follow up FR to implement "publish" of LOI changes (import, delete)?

@jcqli
Copy link

jcqli commented Jul 2, 2024

Filed #1895 to track the longer fix

@gino-m
Copy link
Collaborator Author

gino-m commented Jul 30, 2024

@DaoyuT Has there been any progress on this issue?

@gino-m
Copy link
Collaborator Author

gino-m commented Jul 30, 2024

@sufyanAbbasi, @rfontanarosa mentioned you'd like to take this one on. Do you have a sense how many days work it would be to fix?

@sufyanAbbasi
Copy link
Contributor

For me, between 3-4 days, mostly just investigation and getting myself acclimated to the codebase.

@sufyanAbbasi
Copy link
Contributor

Having played around with it, I noticed that we could create the job when the "new job" button is pressed, just like when we create the initial job in the survey, as we also preemptively create an initial job during the survey creation flow. Then we'll have a Job ID available to bind the uploaded LOIs to?

@gino-m
Copy link
Collaborator Author

gino-m commented Jul 31, 2024

Having played around with it, I noticed that we could create the job when the "new job" button is pressed, just like when we create the initial job in the survey, as we also preemptively create an initial job during the survey creation flow. Then we'll have a Job ID available to bind the uploaded LOIs to?

Unfortunately that would trigger an update to clients of a new job being added before the user actually clicks "Publish". I think the easiest workaround is to warn the user that importing LOIs will cause the job to be immediately published.

@sufyanAbbasi
Copy link
Contributor

I see, that makes sense. On further notice, I found that onImportLocationsOfInterest() which executes on "Upload" seems to use the DataImportService rather than the DraftSurveyService, seems like we're bypassing the mechanism that handles this stuff. Maybe a little bit of refactoring there to make DraftSurveyService more LOI aware can get us most of the way there?

@gino-m
Copy link
Collaborator Author

gino-m commented Aug 1, 2024

@sufyanAbbasi, @jcqli, just heard back from @DaoyuT - he plans to take a look at this next week. I think he already has a workaround in mind, so might be better to wait for him?

DaoyuT added a commit that referenced this issue Aug 9, 2024
… for new unpublished job]

Show LOIs with missing jobs when showPredefinedLoisOnly is true and render with the given selectedJob from the input
@DaoyuT
Copy link
Contributor

DaoyuT commented Aug 9, 2024

Fixed by PR #1958
Opened bug #1964 for future improvement

@DaoyuT DaoyuT closed this as completed Aug 9, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Ground Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

5 participants