feature/testingCenterToKotlin -> dev

This change is

Reviewed 7 of 26 files at r1.
Review status: 7 of 26 files reviewed at latest revision, all discussions resolved.

a discussion (no related file):
Change package name from "testingcenter" to "testingCenter" to match all other features (this may be difficult to do with git, because it doesn't handle case changes well. Sometimes I've had to switch first to something like "testingcenter2", which git does track, and then switch to "testingCenter" so that git realizes that there was a change)

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 31 at r1 (raw file):

class ScheduleFragment: ByuRecyclerViewFragment() {

	private val adapter = HoursAdapter(null)

No need to pass null in anymore (default it)

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 33 at r1 (raw file):

	private val adapter = HoursAdapter(null)

	override fun getTitle(): String {

inline function

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 34 at r1 (raw file):

	override fun getTitle(): String {
		return "Testing Center Hours"

Put in strings.xml

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 49 at r1 (raw file):

		enqueueCall(TestingCenterScheduleClient().getApi(activity).hours, object: ByuCallback<Hours>(this) {
			override fun onSuccess(call: Call<Hours>, response: Response<Hours>) {
				val today = DateUtil.DAY_OF_WEEK_FORMAT.format(Date())

You can probably make an extension on the Date class that will do the formatting instead. Look into it

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 52 at r1 (raw file):

				val hours = response.body()
				val centerHours = hours?.centerHours?.sorted()

inline centerHours if only used once

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 54 at r1 (raw file):

				val centerHours = hours?.centerHours?.sorted()
				// Set the opening and closing times for each day based on which CenterHourChange it is contained in
				centerHours?.let { i ->

No need to name it i. It is by default called it. Use it unless you feel like it needs a better name. (Or in this instance where you have a nested, it might be good to name them. You can also chain a forEach onto centerHours instead of a lets to simplify things

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 102 at r1 (raw file):

		private fun formatHoursDate(date: Date?): String {
			return if (date == null) "Closed" else format.format(date)

strings.xml

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 106 at r1 (raw file):

	}

	companion object {

move to top

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 27 at r1 (raw file):

		var rows: List<DefaultRow> = ArrayList()
		if (data.studentTest.exitDt != null) { // (if they finished the test)

I noticed that you use this logic multiple times. It might be smart to create a really simple computed variable on the studentTest object:
val finished = exitDt != null

or something like that. You could also look into a started var if you think it would help

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 29 at r1 (raw file):

		if (data.studentTest.exitDt != null) { // (if they finished the test)
			rows = Arrays.asList(
					DefaultRow("Test", data.schedule.shortDescription),

strings.xml

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 41 at r1 (raw file):

		if (data.studentTest.exitDt != null && data.studentTest.entryDt != null) {
			val milliseconds = data.studentTest.exitDt.time - data.studentTest.entryDt.time
			return String.format(Locale.US, "%01dh %02dm", TimeUnit.MILLISECONDS.toHours(milliseconds), TimeUnit.MILLISECONDS.toMinutes(milliseconds) % 60)

What is the % doing here? Add a comment

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 54 at r1 (raw file):

	}

	override fun getEmptyDataText(): String {

single line function -> computed function

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 58 at r1 (raw file):

	}

	companion object {

Move to top

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 34 at r1 (raw file):

			{ data, _, _ -> startActivity(Intent(activity, ScoreActivity::class.java).putExtra(STUDENT_TEST_AND_SCHEDULE, data)) }
	)
	private var testingCenterApi: TestingCenterScoresClient.Api? = null

This seems like a lot of instance variables. Make sure all of these actually need to be saved at the class level

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 34 at r1 (raw file):

			{ data, _, _ -> startActivity(Intent(activity, ScoreActivity::class.java).putExtra(STUDENT_TEST_AND_SCHEDULE, data)) }
	)
	private var testingCenterApi: TestingCenterScoresClient.Api? = null

We can make this one a lateinit and make it not nullable (since we're creating ii in the onCreate, and never using it before then). Have you used lateinit before? If not, look up the documentation. It's really nice. Use it anywhere you think prudent

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 60 at r1 (raw file):

	override fun getTitle(): String {
		return "View Scores"

strings.xml (all hard coded strings should go in there, so that we can more easily translate into other languages)

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 66 at r1 (raw file):

		val view = super.onCreateView(inflater, container, R.layout.testing_center_fragment_scores)

		setAdapter(scoresAdapter, true)

Why do we pass in true here? Aren't we still waiting for data to come back?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 67 at r1 (raw file):

		setAdapter(scoresAdapter, true)
		swipeRefreshLayout = view.testing_center_refresh

make testing_center_refresh camel case (all xml ids will not be camel case with our new kotlin extension. Double check everywhere else you've changed)

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 70 at r1 (raw file):

		swipeRefreshLayout?.isEnabled = (result != null) // This disables refresh if the spinner hasn't been populated yet
		swipeRefreshLayout?.setOnRefreshListener {
			spinner?.let { loadScores((it.selectedItem as YearTerm).yearTerm) }

Could this give us a class cast exception? It is possible that the selectedItem would not be a YearTerm? If so, use as?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 78 at r1 (raw file):

		inflater.inflate(R.menu.dropdown_spinner_menu, menu)

		spinner = menu.findItem(R.id.dropdown_spinner_menu_item).actionView as Spinner?

camel case id

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 80 at r1 (raw file):

		spinner = menu.findItem(R.id.dropdown_spinner_menu_item).actionView as Spinner?
		spinner?.let {
			it.onItemSelectedListener = object: AdapterView.OnItemSelectedListener {

Is there a SimpleOnItemSelectedListener or something that would make it so that we don't have to override the onNothingSelected? If not, would it be worth making one?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 82 at r1 (raw file):

			it.onItemSelectedListener = object: AdapterView.OnItemSelectedListener {
				override fun onItemSelected(parent: AdapterView<*>, view: View?, position: Int, id: Long) {
					loadScores((it.getItemAtPosition(position) as YearTerm).yearTerm)

Same as above

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 90 at r1 (raw file):

		}

		makeCall = false // Don't do anything in loadScores(), we'll have the scores either from the previous response or from loadInitialData()

Is there a way that we can compute this variable from other state variables? (such as if the adapter is empty or something)?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 102 at r1 (raw file):

		swipeRefreshLayout?.isRefreshing = true
		testingCenterApi?.let {
			enqueueCall(it.getScores(null), object: ByuCallback<ScoresResponseWrapper>(this) {

Why are we passing in null? Because it defaults to the most recent? If so, make that a defaulted parameter with a comment in the client

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 104 at r1 (raw file):

			enqueueCall(it.getScores(null), object: ByuCallback<ScoresResponseWrapper>(this) {
				override fun onSuccess(call: Call<ScoresResponseWrapper>, response: Response<ScoresResponseWrapper>) {
					result = response.body()

inline result

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 115 at r1 (raw file):

				override fun onFailure(call: Call<ScoresResponseWrapper>, byuError: ByuError) {
					(activity as ByuActivity).showErrorDialog(byuError.readableMessage)

Doesn't ByuFragment have a showErrorDialog function as well? Can't you just do<EMAIL_ADDRESS>If so, use this everywhere that you did the cast before

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 132 at r1 (raw file):

	private fun showScores() {
		val studentTests = result?.content?.studentTests?.sorted()

inline

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 134 at r1 (raw file):

		val studentTests = result?.content?.studentTests?.sorted()
		scoresAdapter.list = studentTests
		swipeRefreshLayout?.isRefreshing = false

Normally, I keep all swipeRefreshLayout refreshing logic in the area where I make the call (isRefreshing = true right before I call, and isRefreshing = false right when it comes back. It makes it easier for someone not to forget). Do this everywhere you find it prudent

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 140 at r1 (raw file):

		super.onAttach(context)

		if (updateOnAttach) {

It seems that this variable is just a semaphore that makes sure we only update the spinner once. Is this correct? If so, is there an attached variable already saved in the fragment? Look into best solutions

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 157 at r1 (raw file):

	private fun updateSpinnerAdapter(context: Context) {
		result?.content?.let {
			SpinnerUtils.setUpAdapter(context, spinner, it.yearTermsWithDescriptions, null)

We can probably move this logic from SpinnerUtils into an extension on the Spinner class. Let me know what you think/find

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 11 at r1 (raw file):

 * Created by cwoodfie on 8/1/16
 */
open class CenterHour: Comparable<CenterHour> {

Why is this an open class? Does it need to be extended?

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 11 at r1 (raw file):

 * Created by cwoodfie on 8/1/16
 */
open class CenterHour: Comparable<CenterHour> {

Why is this not a data class?

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 17 at r1 (raw file):

	var centerCloses: Date? = null

	constructor()

Do we need this empty constructor? Was this auto generated by Android Studio? If so, let's not use the auto-generate anymore, as it seems to miss a number of Kotlin improvements

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 27 at r1 (raw file):

	override fun compareTo(other: CenterHour): Int {
		return CompareUtils.compareInts(DateUtil.getIntValueOfDay(day), DateUtil.getIntValueOfDay(other.day))

I don't think we'll have to use CompareUtils anymore. Look at what it's doing and try to figure out if Kotlin already has that logic

Comments from Reviewable

Reviewed 1 of 26 files at r1.
Review status: 8 of 26 files reviewed at latest revision, 35 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 17 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do we need this empty constructor? Was this auto generated by Android Studio? If so, let's not use the auto-generate anymore, as it seems to miss a number of Kotlin improvements

Oh yeah. You will need the empty constructor, but you can create that by adding () to the class name CenterHour(). Ask Geoff for details. I think he's done it a couple times. Actually, any kotlin model class should have it.

Comments from Reviewable

Reviewed 18 of 26 files at r1.
Review status: all files reviewed at latest revision, 35 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHourChange.kt, line 8 at r1 (raw file):

 * Created by cwoodfie on 8/1/16
 */
class CenterHourChange: CenterHour() {

Use data classes and secondary constructors if needed. Instead of commenting on every object, just leave this one open until you've changed all the model classes to follow the other kotlin classes we've been doing

app/src/main/java/edu/byu/suite/features/testingcenter/model/scores/StudentTestAndSchedule.kt, line 11 at r1 (raw file):

 * Created by cwoodfie on 8/2/16
 */
class StudentTestAndSchedule private constructor(input: Parcel): Comparable<StudentTestAndSchedule>, Parcelable {

ByuParcelable and remove the describeContents. Actually, look at this link, understand it, and try to implement it. Also, if you do, make a IntelliJ Live Template that will make auto complete really easy for it as well. https://medium.com/@BladeCoder/reducing-parcelable-boilerplate-code-using-kotlin-741c3124a49a

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScheduleClient.kt, line 34 at r1 (raw file):

	override fun getConverter(): Converter.Factory {
		// This attempts to parse dates from the different formats we pass in
		val gson = GsonBuilder().registerTypeAdapter(Date::class.java, JsonDateDeserializer(Arrays.asList<SimpleDateFormat>(DateUtil.DATE_AND_TIME_FORMAT, DateUtil.TIME_FORMAT)))

inline gson and just use new lines wherever you think to make things look nice and clean

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScheduleClient.kt, line 36 at r1 (raw file):

		val gson = GsonBuilder().registerTypeAdapter(Date::class.java, JsonDateDeserializer(Arrays.asList<SimpleDateFormat>(DateUtil.DATE_AND_TIME_FORMAT, DateUtil.TIME_FORMAT)))
				// The json we get has underscores in some objects and camelcase in others. this takes care of that
				.setFieldNamingStrategy(ConditionalUnderscoreFieldNamingStrategy(HashSet(Arrays.asList(CenterHour::class.java.canonicalName,

listOf instead of Arrays.asList. In fact, there might be a setOf... Look into it

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScheduleClient.kt, line 42 at r1 (raw file):

	}

	override fun getApiInterface(): Class<Api> {

shorthand function. Do everywhere

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScoresClient.kt, line 21 at r1 (raw file):

 */
class TestingCenterScoresClient: ByuClient<TestingCenterScoresClient.Api>() {
	override fun getBaseUrl(): String {

shorthand function

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScoresClient.kt, line 25 at r1 (raw file):

	}

	override fun getConverter(): Converter.Factory {

shorthand function

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScoresClient.kt, line 29 at r1 (raw file):

	}

	override fun getApiInterface(): Class<Api> {

shorthand function

Comments from Reviewable

Reviewed 16 of 43 files at r2.
Review status: all files reviewed at latest revision, 43 unresolved discussions.

Comments from Reviewable

Review status: 15 of 80 files reviewed at latest revision, 43 unresolved discussions.

a discussion (no related file):
Previously, romrell4 (Eric Romrell) wrote…
Change package name from "testingcenter" to "testingCenter" to match all other features (this may be difficult to do with git, because it doesn't handle case changes well. Sometimes I've had to switch first to something like "testingcenter2", which git does track, and then switch to "testingCenter" so that git realizes that there was a change)

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 31 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No need to pass null in anymore (default it)

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 33 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
inline function

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 52 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
inline centerHours if only used once

I now use it twice.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 54 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No need to name it i. It is by default called it. Use it unless you feel like it needs a better name. (Or in this instance where you have a nested, it might be good to name them. You can also chain a forEach onto centerHours instead of a lets to simplify things

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 102 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
strings.xml

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 106 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
move to top

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 27 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I noticed that you use this logic multiple times. It might be smart to create a really simple computed variable on the studentTest object:
val finished = exitDt != null

or something like that. You could also look into a started var if you think it would help

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 29 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
strings.xml

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 41 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
What is the % doing here? Add a comment

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 54 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
single line function -> computed function

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 58 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Move to top

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 34 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
We can make this one a lateinit and make it not nullable (since we're creating ii in the onCreate, and never using it before then). Have you used lateinit before? If not, look up the documentation. It's really nice. Use it anywhere you think prudent

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 60 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
strings.xml (all hard coded strings should go in there, so that we can more easily translate into other languages)

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 66 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why do we pass in true here? Aren't we still waiting for data to come back?

This is done so that it switches view if the list isn't empty anymore.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 67 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
make testing_center_refresh camel case (all xml ids will not be camel case with our new kotlin extension. Double check everywhere else you've changed)

We talked about IDs and decided to make them underscores.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 70 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Could this give us a class cast exception? It is possible that the selectedItem would not be a YearTerm? If so, use as?

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 78 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
camel case id

See above

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 80 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Is there a SimpleOnItemSelectedListener or something that would make it so that we don't have to override the onNothingSelected? If not, would it be worth making one?

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 82 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Same as above

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 90 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Is there a way that we can compute this variable from other state variables? (such as if the adapter is empty or something)?

I've looked into this and I couldn't find anything that would really work. We can't just check if the adapter is empty because sometimes a semester returns an empty list. This is here because the onCreateOptionsMenu is called whenever the fragment is shown so it was making calls every time you returned from the other fragments in testing center. This variable just ensures that the calls aren't made every time they return. It really does seem like there should be another way to do this but I haven't been able to figure it out.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 102 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why are we passing in null? Because it defaults to the most recent? If so, make that a defaulted parameter with a comment in the client

So this was used to get a list of all the semesters the student has been registered in. I decided to add a method to the client called getSemesters to make this a little easier to understand.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 104 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
inline result

result is used throughout the activity

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 115 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Doesn't ByuFragment have a showErrorDialog function as well? Can't you just do<EMAIL_ADDRESS>If so, use this everywhere that you did the cast before

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 132 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
inline

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 134 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Normally, I keep all swipeRefreshLayout refreshing logic in the area where I make the call (isRefreshing = true right before I call, and isRefreshing = false right when it comes back. It makes it easier for someone not to forget). Do this everywhere you find it prudent

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 140 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
It seems that this variable is just a semaphore that makes sure we only update the spinner once. Is this correct? If so, is there an attached variable already saved in the fragment? Look into best solutions

From what I understood from looking through it I think this is here because once in the logs it showed that the getActivity() call in one of the methods returned null causing a null pointer exception. So what they did was they had it check if the activity was null and if it was it set the updateOnAttach to true and then when ran the OnAttach method it would update the spinner if it needed to. I don't know why this would fix anything because everything is called after the onCreate which is called after the onAttach so the context should never be null. I'm kind of thinking that this didn't actually help anything so I removed it but let me know if you understand why it would be helpful. Removing it hasn't caused any problems in my testing so far.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 11 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why is this an open class? Does it need to be extended?

It's extended by CenterHourChange.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 11 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why is this not a data class?

It is extended by CenterHourChange. From what i've read you cannot extend a data class.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 17 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Oh yeah. You will need the empty constructor, but you can create that by adding () to the class name CenterHour(). Ask Geoff for details. I think he's done it a couple times. Actually, any kotlin model class should have it.

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 27 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I don't think we'll have to use CompareUtils anymore. Look at what it's doing and try to figure out if Kotlin already has that logic

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHourChange.kt, line 8 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Use data classes and secondary constructors if needed. Instead of commenting on every object, just leave this one open until you've changed all the model classes to follow the other kotlin classes we've been doing

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/model/scores/StudentTestAndSchedule.kt, line 11 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
ByuParcelable and remove the describeContents. Actually, look at this link, understand it, and try to implement it. Also, if you do, make a IntelliJ Live Template that will make auto complete really easy for it as well. https://medium.com/@BladeCoder/reducing-parcelable-boilerplate-code-using-kotlin-741c3124a49a

I think I did this right. The main question I have is where to put the parcelableCreator method.

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScheduleClient.kt, line 34 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
inline gson and just use new lines wherever you think to make things look nice and clean

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScheduleClient.kt, line 36 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
listOf instead of Arrays.asList. In fact, there might be a setOf... Look into it

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScheduleClient.kt, line 42 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
shorthand function. Do everywhere

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScoresClient.kt, line 21 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
shorthand function

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScoresClient.kt, line 25 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
shorthand function

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/service/TestingCenterScoresClient.kt, line 29 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
shorthand function

Done.

Comments from Reviewable

Review status: 14 of 80 files reviewed at latest revision, 43 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 34 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Put in strings.xml

In order to do this I had to remove the getTitle function because it was being called before the fragment was attached to the activity which made it so I couldn't call the strings resource file. Now I call getInstance and pass in a title which is then saved in the fragment's arguments. The ByuViewPagerFragment was already built to find the title in the arguments so I think that this is actually how it was originally intended to be used. It also makes it so that it better lines up with the way I implemented the CameraGridFragment.

Comments from Reviewable

Reviewed 62 of 78 files at r3, 4 of 7 files at r4.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

app/src/main/java/edu/byu/suite/features/diningLocations/model/DiningLocation.java, line 75 at r4 (raw file):

	@Override
	public void writeToParcel(Parcel parcel, int flags) {

So I've noticed that this has changed in 5 or 10 files so far. Why the rename? Was this part of the Live Template?

support/src/main/java/edu/byu/support/ByuParcelable.kt, line 9 at r4 (raw file):

 * Created by benjamin on 5/24/16
 */
abstract class ByuParcelable: Parcelable {

Talk to me about how to do the creator

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 34 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
In order to do this I had to remove the getTitle function because it was being called before the fragment was attached to the activity which made it so I couldn't call the strings resource file. Now I call getInstance and pass in a title which is then saved in the fragment's arguments. The ByuViewPagerFragment was already built to find the title in the arguments so I think that this is actually how it was originally intended to be used. It also makes it so that it better lines up with the way I implemented the CameraGridFragment.

So I think getTitle is an abstract function? Did we change that?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 54 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

Nope. Have you used .forEach before? Look into it, and my previous comment will make more sense. Use it to deal with the null checks (you shouldn't need .orEmpty anymore)

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 27 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

No, sorry. Have you used computed variables in Kotlin? It looks more like a function, but you just use an = instead of the { ... }. You would add that to the StudentTest object as a computer variable.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 66 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
This is done so that it switches view if the list isn't empty anymore.

I thought that you only passed in true if you already have all the data loaded. Otherwise, the default false parameter will wait until the data comes back before showing the emptyDataText

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 80 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

It looks like you just removed it... Where did this logic go?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 90 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
I've looked into this and I couldn't find anything that would really work. We can't just check if the adapter is empty because sometimes a semester returns an empty list. This is here because the onCreateOptionsMenu is called whenever the fragment is shown so it was making calls every time you returned from the other fragments in testing center. This variable just ensures that the calls aren't made every time they return. It really does seem like there should be another way to do this but I haven't been able to figure it out.

Is there a difference between null and emptyList?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 102 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
So this was used to get a list of all the semesters the student has been registered in. I decided to add a method to the client called getSemesters to make this a little easier to understand.

But wait... Doesn't this give back semesters and score?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 104 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
result is used throughout the activity

Oh gotcha... Umm... Can you think of a better name that might make it less vague?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 134 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

Not done. Sorry I can't be very explicit when explaining over text. This is how swipeRefresh normally looks
startRefresh
makeCall {
  onSuccess {
    stopRefresh
  }

  onError {
    stopRefresh
  }
}

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 17 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

Whoa... Does this work? I thought you needed to define the empty constructor at the top of this class...

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 27 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

No, I don't think you need to use compareTo. Kotlin allows you to use > and <

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHourChange.kt, line 8 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

Great. So do we not need the empty constructor as well? Does this serialize and unserialize alright?

app/src/main/java/edu/byu/suite/features/testingcenter/model/scores/StudentTestAndSchedule.kt, line 11 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
I think I did this right. The main question I have is where to put the parcelableCreator method.

We can look at it together when I get back

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 24 at r4 (raw file):

	}

	private var finished: Boolean = false

Remove these (see below)

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 49 at r4 (raw file):

	private fun getElapsedTime(data: StudentTestAndSchedule): String {
		if (started && finished) {
			val milliseconds = data.studentTest.exitDt!!.time - data.studentTest.entryDt!!.time //Safe to use !! because values aren't null if started and finished are true

No !!. In fact, I can't think of any good reason why we should ever use it. Think you you could do it with lets or ifs instead

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 29 at r4 (raw file):

	companion object {

		fun newInstance(title: String? = null): ScoresFragment {

If every ByuRecyclerViewFragment is going to need this, is there a way to make it so that we don't have to rewrite it for every one?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 32 at r4 (raw file):

			val fragment = ScoresFragment()
			// Set title for the viewPager
			val args = ByuFragment.getArgsBundleWithTitle(title)

You shouldn't need to prefix this with ByuFragment (because we extend ByuFragment). Also, inline args.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 85 at r4 (raw file):

		inflater.inflate(R.menu.dropdown_spinner_menu, menu)

		spinner = menu.findItem(R.id.dropdown_spinner_menu_item).actionView as Spinner?

as? Spinner, not as Spinner?. Do you understand the difference? as does a hard cast (could throw an error). as? does a soft cast (returns null instead of an error).

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 91 at r4 (raw file):

			loadInitialData()
		} else { // When you return to the ScoresFragment from a different fragment in the activity
			updateSpinnerAdapter(activity)

Why did you start passing the activity in? Can't you get the activity inside the function?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 128 at r4 (raw file):

		scoresAdapter.list = result?.content?.studentTests?.sorted()
		swipeRefreshLayout?.isRefreshing = false
		swipeRefreshLayout?.isEnabled = true

What is the different between enabled and refreshing? Why do we need both?

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/Content.kt, line 9 at r4 (raw file):

 */
data class Content (
	val currentYearTerm: String? = null,

I didn't think we were allowed to use vals here because gson uses the setters in order to load it up... Could you look into that?

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/Schedule.kt, line 9 at r4 (raw file):

import edu.byu.support.ByuParcelable
import edu.byu.support.parcelableCreator

Is parcelableCreator a class? If so, use capital P

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/StudentTestAndSchedule.kt, line 23 at r4 (raw file):

		get() = DateUtil.getElapsedTimeSince(studentTest.exitDt, "Taken %s ago")

	override fun compareTo(other: StudentTestAndSchedule): Int = compareValues(other.studentTest.entryDt, studentTest.entryDt)

What is compare values? Can't we just override the < and >? Google Kotlin comparison

app/src/main/java/edu/byu/suite/features/testingCenter/service/TestingCenterScoresClient.kt, line 28 at r4 (raw file):

	interface Api {
		@GET(".")

Whoa. What does the . do? I think we still need v1

Comments from Reviewable

Review status: 75 of 80 files reviewed at latest revision, 29 unresolved discussions.

app/src/main/java/edu/byu/suite/features/diningLocations/model/DiningLocation.java, line 75 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
So I've noticed that this has changed in 5 or 10 files so far. Why the rename? Was this part of the Live Template?

I changed the name because I thought it made more sense. I didn't realize how many files it would change. When I ran the Kotlin Converter the first time it had set them all to dest.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 34 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
So I think getTitle is an abstract function? Did we change that?

No, its a public method in the ByuFragment class.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 54 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Nope. Have you used .forEach before? Look into it, and my previous comment will make more sense. Use it to deal with the null checks (you shouldn't need .orEmpty anymore)

So I did try it with the .forEach and I didn't like it as much and some people online were saying that using the normal for loops in slightly more memory efficient. But i've switched it back. I think i'm just still getting over my Java habits.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoreActivity.kt, line 27 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No, sorry. Have you used computed variables in Kotlin? It looks more like a function, but you just use an = instead of the { ... }. You would add that to the StudentTest object as a computer variable.

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 66 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I thought that you only passed in true if you already have all the data loaded. Otherwise, the default false parameter will wait until the data comes back before showing the emptyDataText

Oh yeah you're right. Its been switched and tested.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 80 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
It looks like you just removed it... Where did this logic go?

I passed it in to the SpinnerUtils.setUpAdapter method. I still need to work on changing it to Kotlin extensions.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 102 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
But wait... Doesn't this give back semesters and score?

Oh yeah it does. Sorry when I was running curl requests it didn't return any test scores so I thought it only sent back semesters but it really was because I don't have any tests this semester. I've implemented it with the default parameter of null.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 24 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove these (see below)

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 49 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No !!. In fact, I can't think of any good reason why we should ever use it. Think you you could do it with lets or ifs instead

So, while I agree that the !! should very rarely be used (in fact, this is the first time i've tried to justify using it), I think this scenario is a perfect one to use it. I suggest that we either use the !! or we get rid of the finished and started variables. The reason is that the started and finished variables are assigned by checking if data.studentTest.exitDt != null and if data.studyTest.enterDt != null. Performing null safety checks requires us to check exactly the same thing again, wether it is in a double let statement or an if statement, removing any advantage of calculating the started and finished variable. If you are completely opposed to the !! then I suggest we don't use the started or finished variables because that allows us to do the null check at the same time as checking if they have started and finished. I've made the change to better illustrate my point. My preference would be to get rid of the variables.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 85 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
as? Spinner, not as Spinner?. Do you understand the difference? as does a hard cast (could throw an error). as? does a soft cast (returns null instead of an error).

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 91 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why did you start passing the activity in? Can't you get the activity inside the function?

Done.

Comments from Reviewable

Review status: 75 of 80 files reviewed at latest revision, 29 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 104 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Oh gotcha... Umm... Can you think of a better name that might make it less vague?

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 17 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Whoa... Does this work? I thought you needed to define the empty constructor at the top of this class...

It does seem to work.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 27 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No, I don't think you need to use compareTo. Kotlin allows you to use > and <

So < and > return a boolean. Do you want me to put in the logic to return the correct integer? Or should I just use the Kotlin compareTo function? Maybe i'm not understanding....

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHourChange.kt, line 8 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Great. So do we not need the empty constructor as well? Does this serialize and unserialize alright?

Yes it seems to work.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 32 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
You shouldn't need to prefix this with ByuFragment (because we extend ByuFragment). Also, inline args.

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/Content.kt, line 9 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I didn't think we were allowed to use vals here because gson uses the setters in order to load it up... Could you look into that?

In my testing it loads from json just fine....

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/StudentTestAndSchedule.kt, line 23 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
What is compare values? Can't we just override the < and >? Google Kotlin comparison

Um...well > and < returns a boolean and compareValues returns and integer which is required for sorting I think...maybe i'm not understanding. I looked at this link: http://kotlinlang.org/docs/reference/operator-overloading.html#comparison

app/src/main/java/edu/byu/suite/features/testingCenter/service/TestingCenterScoresClient.kt, line 28 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Whoa. What does the . do? I think we still need v1

I understood how the endpoints worked incorrectly, i've changed it now. Sorry about that.

Comments from Reviewable

Review status: all files reviewed at latest revision, 17 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 54 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Haha. You and everyone :) I agree with you that a simple for each loop is still my go to, but when working with an optional list, forEach handles it much better than having to always use .orEmpty. So that's when I will normally use the forEach

Done.

Comments from Reviewable

Review status: 70 of 87 files reviewed at latest revision, 16 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScheduleFragment.kt, line 49 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
You can probably make an extension on the Date class that will do the formatting instead. Look into it

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 66 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Great. I believe false is default, so you shouldn't need to explicitly pass it in. Remove the parameter

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 102 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Awesome. Add a comment in the client (above the method) that explains that passing in null will return the current semester's tests

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 134 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Not done. Sorry I can't be very explicit when explaining over text. This is how swipeRefresh normally looks
startRefresh
makeCall {
  onSuccess {
    stopRefresh
  }

  onError {
    stopRefresh
  }
}
</blockquote></details>

Done.

---

*[app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 157 at r1](https://reviewable.io/reviews/byu-oit/android-byu-suite-v2/547#-LB_Tyy90isMtqmbvK3S:-LCZs461B0W4YaPOPP5f:b-x81p4m) ([raw file](https://github.com/byu-oit/android-byu-suite-v2/blob/956337c48ebb91ec62f32e7ab3078aa677a7d9e8/app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt#L157)):*
<details><summary><i>Previously, romrell4 (Eric Romrell) wrote…</i></summary><blockquote>

We can probably move this logic from SpinnerUtils into an extension on the Spinner class. Let me know what you think/find
</blockquote></details>

I moved this setUpAdapter to the spinner extensions successfully. I still used the SpinnerUtils.getListener because it needed to be a static method and Kotlin doesn't currently support static method extensions on Java classes. All the work arounds I looked up basically created utils classes so I decided to keep it how it was instead of creating a new class in Kotlin as a second spinner utils class. The static method issue is pretty common and many people think that Kotlin will be adding support to it soon.

---

*[app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 17 at r1](https://reviewable.io/reviews/byu-oit/android-byu-suite-v2/547#-LB_UCg-4-cBCGxT7gPl:-LCZtJNeEZ4MM4IYtEad:bd677m2) ([raw file](https://github.com/byu-oit/android-byu-suite-v2/blob/956337c48ebb91ec62f32e7ab3078aa677a7d9e8/app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt#L17)):*
<details><summary><i>Previously, romrell4 (Eric Romrell) wrote…</i></summary><blockquote>

Great. Check with Geoff because I think he did something a little uglier for some of his Kotlin classes. Normalize to whichever is simplest
</blockquote></details>

I talked to Geoff and we weren't sure which classes you were referring to. The only differences we found was how we did the spacing on the data classes so he is changing that. Is there something else you were referring to?

---

*[app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 49 at r4](https://reviewable.io/reviews/byu-oit/android-byu-suite-v2/547#-LCDBUmbBol4Bfv1Vjsq:-LCGZP6eBsDk2KRgvrXB:b-896fix) ([raw file](https://github.com/byu-oit/android-byu-suite-v2/blob/5e18dfc2469f6f1073b62befdebcde6fe996d3d8/app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt#L49)):*
<details><summary><i>Previously, romrell4 (Eric Romrell) wrote…</i></summary><blockquote>

Ah. I think I understand. So you're saying that without the variables, the smart compiler will be able to convert to non-optionals, right? If that's the case, I agree with you and don't use started/finished anymore.
</blockquote></details>

Done.

---

*[app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 29 at r4](https://reviewable.io/reviews/byu-oit/android-byu-suite-v2/547#-LCDCYFh5OqOCIV4r2jp:-LCVXu3o5tDdR7_ieUOM:bqtlkrh) ([raw file](https://github.com/byu-oit/android-byu-suite-v2/blob/5e18dfc2469f6f1073b62befdebcde6fe996d3d8/app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt#L29)):*
<details><summary><i>Previously, romrell4 (Eric Romrell) wrote…</i></summary><blockquote>

If every ByuRecyclerViewFragment is going to need this, is there a way to make it so that we don't have to rewrite it for every one?
</blockquote></details>

This is what I did in the end. Do you want me to go through and change all the classes that override getTitle title to use this now or should we do that on another branch?

---

*[app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 128 at r4](https://reviewable.io/reviews/byu-oit/android-byu-suite-v2/547#-LCDF3yY9my2oIcz3C7U:-LCVZhiOER41G47U34ag:b-reywml) ([raw file](https://github.com/byu-oit/android-byu-suite-v2/blob/5e18dfc2469f6f1073b62befdebcde6fe996d3d8/app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt#L128)):*
<details><summary><i>Previously, romrell4 (Eric Romrell) wrote…</i></summary><blockquote>

What is the different between enabled and refreshing? Why do we need both?
</blockquote></details>

Enabling is what allows you to actually perform the swipe action. We use this to make it so that people can't refresh if the spinner didn't load. Refreshing is what we call to show/dismiss the loading bar itself. I've tested it without any of the enable/disable stuff and it seemed to work just fine without it so I removed it. I think they put it in to prevent someone from trying to refresh before the initial load was completed but because the loading bar comes up anyway it is already disabled.

---

*Comments from [Reviewable](https://reviewable.io/reviews/byu-oit/android-byu-suite-v2/547)*
<!-- Sent from Reviewable.io -->

Review status: 70 of 87 files reviewed at latest revision, 16 unresolved discussions.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 72 at r6 (raw file):

	val today = Date().getDayOfWeek()

	val daysOfWeek = arrayListOf("Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday")

I didn't make this a constant in the end because it is only used in this function.

Comments from Reviewable

Reviewed 17 of 26 files at r6.
Review status: 86 of 87 files reviewed at latest revision, 9 unresolved discussions.

support/src/main/java/edu/byu/support/fragment/ByuFragment.kt, line 18 at r6 (raw file):

 * Created by benjamin on 5/12/16
 */
open class ByuFragment: Fragment(), ByuCallManager {

Since we used the automated converter, look through ever line and make sure you understand everything. I don't want us to have to maintain code that we don't understand

support/src/main/java/edu/byu/support/fragment/ByuFragment.kt, line 58 at r6 (raw file):

	}

	override fun removeCall(call: Call<*>) {

Do we need these wildcards?

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 38 at r6 (raw file):

}

fun Date.isBetween(startDate: Date?, endDate: Date?): Boolean {

Could you validate that this matches the old logic

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 42 at r6 (raw file):

}

fun Date.getDayOfWeek(): String {

Instead of doing this, do you think it would help to just have a .format function that you can pass in the format? Maybe we still keep this for convenience, but it would just pass through?

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 72 at r6 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
I didn't make this a constant in the end because it is only used in this function.

Should this be in the strings.xml as an array?

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 79 at r6 (raw file):

		return (indexOfDay - indexOfToday + 7) % 7
	} else {
		throw IllegalArgumentException("$this is not a valid day of the week")

Strings.xml

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 83 at r6 (raw file):

}

fun <DataType> Spinner.setUpAdapter(context: Context, list: List<DataType>?, onItemSelectedListener: AdapterView.OnItemSelectedListener?): ArrayAdapter<DataType> {

Talk with me about this so that I can understand a little better :)

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 17 at r6 (raw file):

import edu.byu.support.retrofit.ByuCallback
import edu.byu.support.retrofit.ByuError
import edu.byu.support.utils.*

Do we still need this? What utils are we still using?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 57 at r6 (raw file):

					hours.centerHourChange?.forEach { change ->
						if (date?.isBetween(change.beginDate, change.endDate).orFalse()) {

Make sure that this follows the same inclusive/exclusive logic as the old util function

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 29 at r4 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
This is what I did in the end. Do you want me to go through and change all the classes that override getTitle title to use this now or should we do that on another branch?

Another branch will be great. Then you can move all the hard coded strings to strings.xml on that as well

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 72 at r5 (raw file):

		setAdapter(scoresAdapter, false)
		swipeRefreshLayout = view.testing_center_refresh
		swipeRefreshLayout?.isEnabled = (testData != null) // This disables refresh if the spinner hasn't been populated yet

Was this removed on purpose? Not needed?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/TestingCenterMainActivity.kt, line 31 at r6 (raw file):

				fragmentManager,
				arrayOf<ByuFragment>(
						ByuFragment.newInstance<ScoresFragment>(getString(R.string.view_scores)),

Does it not work to use ScoresFragment.newInstance(...)?

Comments from Reviewable

Reviewed 1 of 78 files at r3.
Review status: all files reviewed at latest revision, 19 unresolved discussions.

Comments from Reviewable

Review status: 83 of 87 files reviewed at latest revision, 19 unresolved discussions.

support/src/main/java/edu/byu/support/fragment/ByuFragment.kt, line 18 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Since we used the automated converter, look through ever line and make sure you understand everything. I don't want us to have to maintain code that we don't understand

Done.

support/src/main/java/edu/byu/support/fragment/ByuFragment.kt, line 58 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do we need these wildcards?

We talked about this and decided they were necessary.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 38 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Could you validate that this matches the old logic

Done.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 42 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Instead of doing this, do you think it would help to just have a .format function that you can pass in the format? Maybe we still keep this for convenience, but it would just pass through?

Done.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 72 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Should this be in the strings.xml as an array?

We can't access strings.xml from Kotlin extension without passing in context.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 79 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Strings.xml

See above

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 34 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
This seems like a lot of instance variables. Make sure all of these actually need to be saved at the class level

I spent some time trying to figure out how to reduce the number of instance variables but I couldn't come up with anything reasonable.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 17 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do we still need this? What utils are we still using?

I was using a ViewUtils but that has been fixed. Also, the Kotlin extensions require it because they are in the support folder.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 57 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Make sure that this follows the same inclusive/exclusive logic as the old util function

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 72 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Was this removed on purpose? Not needed?

Yes. I tested it without the isEnabled and it worked fine. I'm pretty sure it was there to prevent people from trying to refresh while the spinner was still loading but the loading bar is showing anyway disabling the swipe to refresh.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/TestingCenterMainActivity.kt, line 31 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Does it not work to use ScoresFragment.newInstance(...)?

You can't call parent static methods from a child reference in Kotlin. https://stackoverflow.com/questions/39303180/kotlin-how-can-i-create-a-static-inheritable-function

Comments from Reviewable

Reviewed 9 of 13 files at r8.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 6 at r8 (raw file):

import android.widget.AdapterView

open class SimpleOnItemSelectedListener: AdapterView.OnItemSelectedListener {

Why does this need to be open?

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 7 at r8 (raw file):

open class SimpleOnItemSelectedListener: AdapterView.OnItemSelectedListener {
	override fun onItemSelected(parent: AdapterView<*>?, view: View?, position: Int, id: Long) {}

Why are we overriding this? I thought this was going to be an abstract class, and this was going to be abstract

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 146 at r8 (raw file):

fun Parcel.readIntOpt() = readValue(Int::class.java.classLoader) as? Int

fun Parcel.writeDate(date: Date?) {

inline (like the other ones)

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 123 at r8 (raw file):

		testData?.content?.let {
			spinner?.setUpAdapter(activity, it.yearTermsWithDescriptions, object: SimpleOnItemSelectedListener() {
				override fun onItemSelected(parent: AdapterView<*>?, view: View?, position: Int, id: Long) {

Oh... We can't use a lambda here? That puts a damper on this solution...

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/Schedule.kt, line 15 at r8 (raw file):

data class Schedule(
		@SerializedName("dept_name")
		val deptName: String,

So none of these are nullable? What would happen if the service returned a null?

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/StudentTest.kt, line 34 at r8 (raw file):

	override fun writeToParcel(parcel: Parcel, flags: Int) {
		// We use writeValue instead of writeDouble because percent can be null

Remove comment?

Comments from Reviewable

Review status: 83 of 88 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 6 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why does this need to be open?

See below.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 7 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why are we overriding this? I thought this was going to be an abstract class, and this was going to be abstract

Oh gotcha. Sorry I forgot what the solution was that we came up with when I came in this morning.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 79 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
How does Kotlin deal with this exception? Do we try/catch it in the calling function? Or it is a silent error?

They only have unchecked exceptions so I don't do any try/catch when I call it. I can add them though. https://kotlinlang.org/docs/reference/exceptions.html#exception-classes

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 83 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Talk with me about this so that I can understand a little better :)

Done.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 42 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I don't think this name is correct. Probably something like format would describe it better

Done.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 146 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
inline (like the other ones)

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 90 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Is there a difference between null and emptyList?

Actually the adapter should never have a null list when this is called because it will already be populated by the loadInitialData call so we can't use that to check. This just makes sure that it doesn't make the same call twice. I can walk you through it sometime.

app/src/main/java/edu/byu/suite/features/testingcenter/model/hours/CenterHour.kt, line 27 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
See above. We'll discuss it next week :)

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/model/scores/StudentTestAndSchedule.kt, line 11 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
We can look at it together when I get back

Done.

support/src/main/java/edu/byu/support/ByuParcelable.kt, line 9 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Talk to me about how to do the creator

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 123 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Oh... We can't use a lambda here? That puts a damper on this solution...

So what i've read suggests that you can only do that on interfaces but maybe i'm missing something....

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/Schedule.kt, line 9 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Is parcelableCreator a class? If so, use capital P

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/Schedule.kt, line 15 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
So none of these are nullable? What would happen if the service returned a null?

I made them nullable.

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/StudentTest.kt, line 34 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove comment?

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/model/scores/StudentTestAndSchedule.kt, line 23 at r4 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Oh... super interesting... maybe this doesn't work the way that I thought it did... We can look at this together next week. Below is my previous understanding...
So the compareTo function does require an Int, but instead I believe you can override the </>/= operators (I'm not sure if you need all of them), which return booleans

Done.

Comments from Reviewable

Reviewed 4 of 43 files at r2, 6 of 9 files at r9, 2 of 4 files at r10.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 5 at r9 (raw file):

import android.widget.AdapterView

abstract class SimpleOnItemSelectedListener: AdapterView.OnItemSelectedListener {

Oh I'm so dumb!!! One of the great things that they introduced in Kotlin is interfaces with default implementations! Just make this an interface!
Oh dang, this doesn't actually let you use a lambda for some reason...

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 109 at r10 (raw file):

	val daysOfWeek = arrayListOf("Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday")
	return when (daysOfWeek.contains(this)) {

A when with two option is the same as ternary. Just use ternary here

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 90 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Actually the adapter should never have a null list when this is called because it will already be populated by the loadInitialData call so we can't use that to check. This just makes sure that it doesn't make the same call twice. I can walk you through it sometime.

Sounds good. It sounds like we need to keep it, but walk me through it

app/src/main/java/edu/byu/suite/features/testingcenter/model/scores/StudentTestAndSchedule.kt, line 11 at r1 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

Do these all need to be nullable as well? Check all of your classes to make sure that it will never break

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 54 at r10 (raw file):

				centerHours?.forEach { hour ->
					val date = hour.day?.getNextDateWithDayName()
					hour.day = (if (hour.day == today) getString(R.string.today) else hour.day) + (date?.let { ", " + DATE_FORMAT.format(date) } ?: "")

So how would this look to the user?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 54 at r9 (raw file):

	private fun getScoreValue(data: StudentTestAndSchedule): String {

		return data.studentTest.percent?.let { String.format(Locale.US, "%.1f%%", data.studentTest.percent * 100) }

I don't know if it will look better or not here, but anytime you have a one line function (that just returns), you can use the = notation instead of the {}. Look at this and decide which you think looks better

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 123 at r8 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
So what i've read suggests that you can only do that on interfaces but maybe i'm missing something....

I think you're right. Let's talk about it when you're in

Comments from Reviewable

Review status: 84 of 87 files reviewed at latest revision, 6 unresolved discussions.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 5 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Oh I'm so dumb!!! One of the great things that they introduced in Kotlin is interfaces with default implementations! Just make this an interface!
Oh dang, this doesn't actually let you use a lambda for some reason...

So do you want it to be an interface or an abstract class?

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 109 at r10 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
A when with two option is the same as ternary. Just use ternary here

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/model/scores/StudentTestAndSchedule.kt, line 11 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do these all need to be nullable as well? Check all of your classes to make sure that it will never break

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 54 at r10 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
So how would this look to the user?

It just shows the string returned from the service. For example, if the service were to return "Monday", that would show "Monday, May 13, 2018" because that is a valid string. But if the service returned "Today", that wouldn't be accepted as a valid day and so it would only display "Today". I tested this and it does format as I expected. The only other option I can think of is to just complete skip displaying that information.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 54 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I don't know if it will look better or not here, but anytime you have a one line function (that just returns), you can use the = notation instead of the {}. Look at this and decide which you think looks better

Hmmm...I'm not sure if this looks better. What do you think?

Comments from Reviewable

Review status: 83 of 88 files reviewed at latest revision, 6 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.kt, line 90 at r1 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Sounds good. It sounds like we need to keep it, but walk me through it

Done.

Comments from Reviewable

Reviewed 5 of 8 files at r11.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 5 at r9 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
So do you want it to be an interface or an abstract class?

what do you think? I would lean towards interface (since it doesn't need any state variables), but I could be persuaded if you can think of anything

support/src/main/java/edu/byu/support/login/LoginActivity.java, line 52 at r11 (raw file):

				OAuthToken token = response.body();
				if (token == null || !token.isValid()) {
					onFailure(call, null); //TODO: this shouldn't be null

Can you fix this real quickly?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 54 at r10 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
It just shows the string returned from the service. For example, if the service were to return "Monday", that would show "Monday, May 13, 2018" because that is a valid string. But if the service returned "Today", that wouldn't be accepted as a valid day and so it would only display "Today". I tested this and it does format as I expected. The only other option I can think of is to just complete skip displaying that information.

Can you check with iOS to make sure this logic matches theirs?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 54 at r9 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Hmmm...I'm not sure if this looks better. What do you think?

throw the String.format onto the same line as the let, and then just have the ?: ... on the next line and tell me if that looks any better

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 54 at r11 (raw file):

	private fun getScoreValue(data: StudentTestAndSchedule): String = data.studentTest?.percent?.let {
			String.format(Locale.US, "%.1f%%", it * 100)
		} ?: if (data.scoreDisplayLevelText == null || data.scoreDisplayLevelText.isEmpty()) "--%" else data.scoreDisplayLevelText

Was adding the == null on purpose? Could you do that with optional chaining?

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 61 at r11 (raw file):

		spinner = menu.findItem(R.id.dropdown_spinner_menu_item).actionView as? Spinner

		makeCall = false // Don't do anything in loadScores(), we'll have the scores either from the previous response or from loadInitialData()

Change comment to match new names

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 69 at r11 (raw file):

	}

	//Null year term, first call

Update comment to make it more helpful (and english-y)

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 93 at r11 (raw file):

	}

	private fun updateSpinnerAdapter() {

should this be named setUpSpinnerAdapter? or is this called to update it as well?

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 79 at r11 (raw file):

				if (yearTerm == null) {
					testData?.content?.let { content ->
						spinnerItemSelected = content.yearTermsWithDescriptions?.indexOfFirst { it.yearTerm == content.currentYearTerm } ?: spinnerItemSelected

Check the docs to see what indexOfFirst returns if it's not found... It might be -1. If so, look for a indexOfFirstOrNull

Comments from Reviewable

Review status: 83 of 87 files reviewed at latest revision, 10 unresolved discussions.

support/src/main/java/edu/byu/support/SimpleOnItemSelectedListener.kt, line 5 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
what do you think? I would lean towards interface (since it doesn't need any state variables), but I could be persuaded if you can think of anything

Done.

support/src/main/java/edu/byu/support/utils/Extensions.kt, line 108 at r12 (raw file):

fun <DataType> Spinner.setUpAdapter(context: Context, list: List<DataType>?, onItemSelectedListener: AdapterView.OnItemSelectedListener?): ArrayAdapter<DataType> {
	val arrayAdapter = ArrayAdapter<DataType>(context, R.layout.byu_simple_spinner_dropdown_item, list.orEmpty())

I added the .orEmpty() because I was getting a null pointer exception when passing in null lists.

support/src/main/java/edu/byu/support/login/LoginActivity.java, line 52 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Can you fix this real quickly?

Geoff is doing this.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 54 at r10 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Can you check with iOS to make sure this logic matches theirs?

From what I can tell from the swift, they don't actually handle it. If the day doesn't exist in the array then it returns 0 from getIntValue in the TestingCenterUtils class. This then adds 0 to the current date which means the date that will be returned is the current date. If I understand this correctly, this means that if the service sends days that aren't valid, the schedule list will show the name returned from the service with the same date which is todays date.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 54 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
throw the String.format onto the same line as the let, and then just have the ?: ... on the next line and tell me if that looks any better

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoreActivity.kt, line 54 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Was adding the == null on purpose? Could you do that with optional chaining?

So the difficulty here is the || logic. It needs to do the same thing if its null or its empty which makes optional chaining a little difficult. Another option would be something like if  (data.scoreDisplayLevelText.isNullOrEmpty()) "--%" else data.scoreDisplayLevelText but the smart compiler doesn't recognize that scoreDisplayLevelText isn't null so I would have to do a null check again. As far as I can tell Kotlin doesn't have a simple way to make an empty list null. If they did then it would be easy.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 123 at r8 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I think you're right. Let's talk about it when you're in

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 61 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Change comment to match new names

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 69 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Update comment to make it more helpful (and english-y)

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 79 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Check the docs to see what indexOfFirst returns if it's not found... It might be -1. If so, look for a indexOfFirstOrNull

There was no indexOfFirstOrNull or anything similar that I could find. Another thing about his part is i've made it so that it will still show the score data if the list of yearTermsWithDescriptions returned from the service is null. It shows the score info but it doesn't populate the spinner. The only other way to handle this that I could think of is to show an error message.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 93 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
should this be named setUpSpinnerAdapter? or is this called to update it as well?

Done.

Comments from Reviewable

Reviewed 5 of 7 files at r12.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 54 at r12 (raw file):

				centerHours?.forEach { hour ->
					val date = hour.day?.getNextDateWithDayName()
					hour.day = (if (hour.day == today) getString(R.string.today) else hour.day) + (date?.let { ", " + DATE_FORMAT.format(date) } ?: "")

Comment

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 79 at r11 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
There was no indexOfFirstOrNull or anything similar that I could find. Another thing about his part is i've made it so that it will still show the score data if the list of yearTermsWithDescriptions returned from the service is null. It shows the score info but it doesn't populate the spinner. The only other way to handle this that I could think of is to show an error message.

Just show the error message so that they have to retry. If the service doesn't return the year terms, we can't use all the functionality of the screen.

Comments from Reviewable

Review status: 85 of 87 files reviewed at latest revision, 2 unresolved discussions.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScheduleFragment.kt, line 54 at r12 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Comment

Done.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 79 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Just show the error message so that they have to retry. If the service doesn't return the year terms, we can't use all the functionality of the screen.

Done.

Comments from Reviewable

Reviewed 2 of 4 files at r13.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 79 at r11 (raw file):
Previously, aarowhead (Aaron Hill) wrote…
Done.

Just throw an error

Comments from Reviewable

Review status: 86 of 87 files reviewed at latest revision, 1 unresolved discussion.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 79 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Just throw an error

Done.

Comments from Reviewable

Reviewed 1 of 2 files at r14.
Review status: all files reviewed at latest revision, all discussions resolved.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 84 at r14 (raw file):

							spinnerItemSelected = yearTerms.indexOfFirst { it.yearTerm == content.currentYearTerm }
							// If the current yearTerm is not a member of the list of yearTerms returned, we throw an error
							if (spinnerItemSelected < 0) showErrorDialog(ERROR_MESSAGE)

Try to combine the showErrorDialog calls

Comments from Reviewable

Review status: 86 of 87 files reviewed at latest revision, 1 unresolved discussion.

app/src/main/java/edu/byu/suite/features/testingCenter/controller/ScoresFragment.kt, line 84 at r14 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Try to combine the showErrorDialog calls

Done.

Comments from Reviewable

Reviewed 1 of 2 files at r15.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


Aync/Await wise Match operations

Make possible to call async methods within the Success callback in Match and MatchFirst methods
ErrorOr<Success> result = SomeOperation();

await result.Match<ErrorOr<Success>>(
   async value => { await DoAsync(value); },
   error => { return error; }
);

Thanks for creating this PR and sorry it took me so long to get to this! This feature is now supported with the Async prefix to each of the match/switch methods thanks to blouflashdb 🙂


v6 connectivity broken upon daemon restart due to failed neigh probes on router address

While testing #9062, I noticed that v6 connectivity gets broken from time to time. Turns out that the reason is the following: 1) we start up the agent, 2) we create a container/pod, 3) in the latter we set a default route with ROUTER_IP. The container is doing neighbor solicitation requests to the ROUTER_IP once it neigh entry becomes stale and needs to be reprobed. 4) daemon restarts. Now the connectivity problems start once the neigh entry became STALE->PROBE->FAILED. Thus container has no neigh entry for the default route and therefore no connectivity anymore for v6. Reason is that upon agent restart ROUTER_IP was newly (randomly) generated, and endpoint programs regenerated. This also means that our ICMP handling code stops replying to neigh requests to the old ROUTER_IP. v4 has not this issue as the internal allocated addr for cilium_host is attempted to be persistent accross restarts, but not the v6 router IP.

Confirmed regression in 1.6.

Regression caused by ae856dda43b4538e567190ec2db9f8a9c54e1c99

Before (picks 2001:db8:5::1):
level=info msg="  IPv6 node prefix: 2001:db8:5::/96" subsys=daemon
level=info msg="  IPv6 allocation prefix: 2001:db8:5::/112" subsys=daemon
level=info msg="  IPv6 router address: 2001:db8:5::1" subsys=daemon
level=info msg="  External-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Internal-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Cluster IPv4 prefix: <IP_ADDRESS>/8" subsys=daemon
level=info msg="  IPv4 allocation prefix: <IP_ADDRESS>/16" subsys=daemon

[ restart ]
level=info msg="  IPv6 node prefix: 2001:db8:5::/96" subsys=daemon
level=info msg="  IPv6 allocation prefix: 2001:db8:5::/112" subsys=daemon
level=info msg="  IPv6 router address: 2001:db8:5::1" subsys=daemon
level=info msg="  External-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Internal-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Cluster IPv4 prefix: <IP_ADDRESS>/8" subsys=daemon
level=info msg="  IPv4 allocation prefix: <IP_ADDRESS>/16" subsys=daemon

After ae856dd (ROUTER_IP is now random after every restart, the internal v4 one not):
level=info msg="  IPv6 node prefix: 2001:db8:5::/96" subsys=daemon
level=info msg="  IPv6 allocation prefix: 2001:db8:5::/112" subsys=daemon
level=info msg="  IPv6 router address: 2001:db8:5::16c3" subsys=daemon
level=info msg="  External-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Internal-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Cluster IPv4 prefix: <IP_ADDRESS>/8" subsys=daemon
level=info msg="  IPv4 allocation prefix: <IP_ADDRESS>/16" subsys=daemon

[ restart ]
level=info msg="  IPv6 node prefix: 2001:db8:5::/96" subsys=daemon
level=info msg="  IPv6 allocation prefix: 2001:db8:5::/112" subsys=daemon
level=info msg="  IPv6 router address: 2001:db8:5::a3a3" subsys=daemon
level=info msg="  External-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Internal-Node IPv4: <IP_ADDRESS>" subsys=daemon
level=info msg="  Cluster IPv4 prefix: <IP_ADDRESS>/8" subsys=daemon
level=info msg="  IPv4 allocation prefix: <IP_ADDRESS>/16" subsys=daemon


Add AirPlay Device Type To Name

This helps the media player be more explicit about itself and what it is. It also namespaces it self a little better in the system.
Rather than be media_player.family_room it is media_player.family_room_apple_tv. This helps for cases when there’s another actual media player like Kodi or Chromecast in there.

I sort of hate this still, because now we have something like: media_player.family_room_apple_tv or media_player.deck_airport_express and leads to confusing expectations.
These aren't actually addressable Apple TVs where we can do what you might think to them. They are only there because they're to be used with the media_player.itunes type that will airplay to them when they're turned on|off.
So I'm sort of torn. Should they be name spaced with itunes-ap in it somewhere or something? Because while they're NOT exclusive to the itunes-api media player (these are actual apple tvs), they ARE directly paired to it in terms of functionality.
 Lots of questions.

Ok I think this is legit. Their names are all just appended with AirTunes Speaker. That gives the expectation of music only, a pairing with iTune (kindaaaaa), and that they're a SPEAKER.
I think this is ready to go now.

This looks good. I wouldn't care too much about entity id's. Those are not visible in the UI anyway.
Only problem that can occur is that when two media players would get the same entity id, one of them (nondeterministic) will have _2 appended.


As a Team We want to setup cloud test environment for Shopware-PWA So we can work and test together

Context
In the beginning of the project we want to setup shared test environment. We want to use Divante's technology for that, which allows us to host that in a cloud.
Acceptance Criteria

Test environment is running in Divante VueStorefront Cloud.
Test environment is available under domain shopware-pwa.storefront.io.
Documentation describes how to access test environment. For now it should be in Confluence: https://divante.atlassian.net/wiki/spaces/SHOP/pages/1479049282/Test+environment
Test environment is not secured by password.
DivanteLtd/shopware-pwa is pulled to test environment.
@patzick can access test environment.
@rmakara can access test environment.
@mkucmus can access test environment.

Test of Slack integration with shopware-pwa Github repository.

Test of Slack integration with shopware-pwa repository.

Test env has been created. Credentials were stored at the confluence.


Normalize audit log attribute names in cluster.yaml + add support for missing attributes

The auditLog.(logpath|maxage) attribute names were normalized to auditLog.(logPath|maxAge), and added support for the maxBackup (which was hard-coded to 1) and maxSize flags.
Also, the cluster.yaml now uses the same default value for auditLog.logPath as the one defined in the struct.

@danielfm LGTM! Thanks again for your contribution.


metrics: Ignore labels with empty values

Workaround for https://github.com/prometheus/pushgateway/issues/344
Prometheus push gateway rejects labels with empty values (e.g. when SGX is not used tee_hardware label is empty). This PR removes labels with empty values before sending them to push gateway.

oasis-core-daily-benchmarks now work back with this fix.


Release 1.0.0

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@72d7f53). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #76   +/-   ##
=========================================
  Coverage          ?   99.59%
  Complexity        ?      594
=========================================
  Files             ?      101
  Lines             ?     1718
  Branches          ?        0
=========================================
  Hits              ?     1711
  Misses            ?        7
  Partials          ?        0

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more


allow return and display of searchResult alerts.

Previously if an invalid query was passed in nothing would be returned or displayed to the user.
E.g using structural search without the correct flag
select * from sourcegraph_search("repo:^github.com/askgitdev/askgit$ if  :[if_statement] { :[matched_if] } else { :[matched_else] }");
Return was previously nil which is not terribly helpful.
Now it returns
{"Alert":{"alertTitle":"No results","alertDescription":"It looks like you may have meant to run a structural search, but it is not toggled.","ProposedQueries":[{"proposedQueryDescription":"Activate structural search","proposedQuery":"repo:^github.com/askgitdev/askgit if :if_statement { :[matched_if] } catch { :[matched_else] } patternType:structural"}]}}
Which is much more actionable

Codecov Report

Merging #171 (c3aedf8) into main (dadb0ba) will decrease coverage by 0.10%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   66.77%   66.66%   -0.11%
==========================================
  Files          41       41
  Lines        1875     1887      +12
==========================================
+ Hits         1252     1258       +6
- Misses        407      413       +6
  Partials      216      216

Impacted Files
Coverage Δ

...ensions/internal/sourcegraph/sourcegraph_search.go
50.61% <50.00%> (-0.11%)
:arrow_down:

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 dadb0ba...c3aedf8. Read the comment docs.


The selector "my-app" did not match any elements

I noticed while running this boilerplate that Angular sometimes complains about the my-app selector not being found.
Potential fix:
I believe the systemjs script that starts up the app should be placed after the  tags. This will ensure that once Angular is bootstrapped, the DOM already has loaded the  element.  Once I did this, my problem subsided.
index.html should look like this:
<body>
<my-app>Loading...</my-app>

<script>
    System.config({
        packages: {
            app: {
                format: 'register',
                defaultExtension: 'js'
            }
        }
    });
    System.import('app/boot')
            .then(null, console.error.bind(console));
</script>
</body>

thoughts?

Sounds reasonable to me - only thing which I find a bit strange is, that with this boilerplate I follow the quickstart guide form angular.io where they place Systemjs BEFORE .
But your explanation makes sense, I'll update it to reflect this.


Feature/beamselector

Added unit tests for panda trigger divider.

Codecov Report

Merging #286 into master will increase coverage by 0.12%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   92.22%   92.34%   +0.12%
==========================================
  Files         206      206
  Lines       10477    10480       +3
==========================================
+ Hits         9662     9678      +16
+ Misses        815      802      -13

Impacted Files
Coverage Δ

...les/ADPandABlocks/parts/pandaalternatingdivpart.py
83.33% <66.66%> (+35.18%)
:arrow_up:

malcolm/modules/pmac/parts/beamselectorpart.py
93.33% <0%> (+6.66%)
:arrow_up:

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 bac116d...8e093be. Read the comment docs.


Cannot specify signed URL TTL in days

What is the feature or enhancement?
I want to type "3d" as the TTL

Days would be useful, currently the ttl option is parsed with time.ParseDuration which does not support days.


[CI] What if Github Actions can handle the split of packages?

@natepage I think you'll love this. I've found a way to use Github Actions to handle the monorepo split :)
It's blazing fast and just works :thinking: :laughing:

Here is the split package: https://github.com/symplify/skipper

Here is the last commit in the split package: https://github.com/symplify/skipper

Full experiment on @migrify monorepo - https://github.com/migrify/migrify/pull/205/files


[BUG] UI issue in Media Details Page

Summary:
The padding in category item, file usage item is not looking good. We need to make them consistent with other layouts.
Steps to reproduce:
Open Media Details Page
Screen-shots:

Would you like to work on the issue?
No.

I would like to work on this issue if it is okay with the team.  In addition, is it possible to update the README with the slack channel details so I can introduce myself?

Hi @ngumbau sure feel free to start working on it. I will update the readme but the slack channel isn't open for everyone so please share your email ID with me. I will ask Sage to send you an invite.

<EMAIL_ADDRESS>

I want to work on this issue, my email id is<EMAIL_ADDRESS>Invite me also.


[ASTGen/Macros] Ensure 'offset' from plugin response is valid

We see some crashes presumably caused by invalid 'offset' returned from macro plugins. Although we haven't been able to find a reproducer, guard not to generate invalid source location by checking 'offfset' is within the buffer length.
rdar://125625879

@swift-ci Please smoke test


The 'wpadmin' admin template defines the Save button as a button element...

Small fix for compatibility with "wpadmin" (and possibly other) Django admin templates, see commit

Thanks!

Merged into next, new release coming very soon.


Move draw controls

Rolling out https://github.com/acteng/scheme-sketcher-lib/pull/6. Still need to update some tests, pushing here to get GH actions to help parallelize them.
Smaller improvements tracked in #536

The 3 failures in https://github.com/acteng/atip/actions/runs/11683808812/job/32533769777?pr=537 are timeouts and not reproducible locally. I've split the files as playwright recommends. If that doesn't work, I'll disable some tests or just merge anyway. Manually testing the functionality and running the tests locally is working fine.

Random tests are now just timing out. I'm going to merge and deal with this later.


Support using other session objects e.g. requests_cache

Refactor cot_reports.py to put functions in new class with session property.
User API not changed - original functions simply pass-through to class instance.
New API function set_session, for user to pass through a different session object - example in README.

@NDelventhal Can you review & merge? I have plans for a bigger PR next - file caching & data cleaning.


Final Website

Checklist
Required changes:

[x] Center the description (Line 17 in HTML file) using a single change in CSS
[x] Fix JS so that when the search is executed, the search engine is not present in query
[x] Add Google Drive, Google Slides and Google Sheets as a provider

Optional changes:

[x] Add other search providers (GitHub, Spotify, etc)
[x] Additional CSS styling (as you like)

Hosted on GitHub Pages

Good work with this, but all your commits change just one file! While atomic commits is a good practice to follow, too many commits can be tedious to deal with