fix: convert "manual" service ids error into changeset error

[firestack]

Summary of changes

Asana Ticket: Show route and service ids missing errors

This PR is aiming to address a few factors

1. I wanted to reduce the use of the :errors assign, so that I didn't have to compete with the error implementation in 🏹🚆 Trainsformer Validations: Make sure all errors are reported to frontend #1354 (see comment)
2. These errors already exist on the changeset, so it seemed appropriate to surface them from the changeset, rather than to implement them as a special case in the update_export function

---

The reason for this PR is that I needed to get rid of this "error", because it's being used in a way that is both handled by Changeset's and doesn't match the way I want to return errors from the export validations, and conveniently, it actually makes it simpler to remove this code.

— #1352 (comment)

---

Before:

After:

Reviewer Checklist

• Meets ticket's acceptance criteria
• Any new or changed functions have typespecs
• Tests were added for any new functionality (don't just rely on Codecov)
• This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

[firestack]

contents:

: unzip -c test/support/fixtures/trainsformer/invalid,reasons=no-trips,no-stop-times,no-multi-route-trips.zip
Archive:  test/support/fixtures/trainsformer/invalid,reasons=no-trips,no-stop-times,no-multi-route-trips.zip
 extracting: multi_route_trips.txt
added_route_id,trip_id

  inflating: stop_times.txt
trip_id,arrival_time,departure_time,stop_id,stop_sequence,stop_headsign,pickup_type,drop_off_type,timepoint,bikes_allowed,nonstandard_track

  inflating: trips.txt
route_id,service_id,trip_id,trip_headsign,trip_short_name,shape_id,direction_id,bikes_allowed,event_time

[firestack]

the reason for this PR is that I needed to get rid of this "error", because it's being used in a way that is both handled by Changeset's and doesn't match the way I want to return errors from the export validations, and conveniently, it actually makes it simpler to remove this code.

[firestack]

previously, if export_params["services"] was empty, this would actually crash and therefore we wouldn't upload the file to S3. So the change to check export_params["services"] does change the behavior. But, we now show the errors when validate runs, which does run after the export Zip is uploaded (demonstrated in the test in this PR).

I think the logic here of crashing the process if the "services" key should be cleaned up and prevent errors, but this PR could move forward without this change, and drop this commit and address this at a later date.

I think the proper way to do this would be to add the zip contents and necessary metadata as virtual fields on the changeset, and then use Ecto.Changeset.prepare_changes to upload to S3 before inserting into the DB, and update and validate the s3_path at that point. Then we never upload a file without the changeset being valid.

[firestack]

Since github didn't show it very well, this is the code I'm talking about

    with {:ok, s3_path} <-
           ExportUpload.upload_to_s3(
             socket.assigns.uploaded_file_data,
             socket.assigns.uploaded_file_name,
             socket.assigns.disruption.id
           ),

[rudiejd]

I think this should be cleaned up and prevent errors, but we can also drop this commit for later.

what is this here?

Personally I'm not super concerned about forcing every error into the changeset - it seems fine to me if we don't even create an export in the DB if the S3 upload fails.

[rudiejd]

but in the case in which we uploaded an invalid file to S3, I agree that it should be cleaned up... I can make a follow-up ticket for that

[rudiejd]

https://app.asana.com/1/15492006741476/project/1211698384535007/task/1213125718572393?focus=true

[firestack]

I think this should be cleaned up and prevent errors, but we can also drop this commit for later.

what is this here?

Apologies, this is referring to the logic that would crash the liveview process if export_params["services"] was not present. I've updated the original comment.

Personally I'm not super concerned about forcing every error into the changeset

I strongly disagree with this, I think we have a place for errors to be, and to have more "error" variables strewn throughout the code makes coming back to the code later less clear and the overall usage of Phoenix weaker. Even if we don't want the errors to be surfaced in this specific way in the UI, I strongly believe we should be doing all (or as many as possible) errors in the changeset itself, rather than ad-hoc.

[firestack]

note: I don't like the "successfully imported" but actually there's errors, but I think we can fix that later/follow up PR? thoughts?

[rudiejd]

I'd prefer something like "Imported export with warnings" if possible. I personally think it would be fine to do that as part of this PR since it should be a relatively small change, but happy to put it off if it ends up being difficult.

[rudiejd]

@firestack could please you provide either acceptance criteria in the ticket or a description of the problem you're trying to solve in the PR? There might be some thread that I'm missing, but these are not immediately evident to me.

[firestack]

@rudiejd Added the reasoning to the description